Re: [PATCH 1/5] Add internal <file_change_detection.h> header file

[Florian Weimer <fw at deneb dot enyo dot de>]

* Adhemerval Zanella:

> On 21/01/2020 15:41, Florian Weimer wrote:
>> The code started out with bits form resolv/resolv_conf.c, but it
>> was enhanced to deal with directories and FIFOs in a more predictable
>> manner.  A test case is included as well.
>> 
>> This will be used to implement the /etc/resolv.conf change detection.
>> 
>> This currently lives in a header file only.  Once there are multiple
>> users, the implementations should be moved into C files.
>
> LGTM, with some nits below.
>
> Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>

Thanks.
>> +static void
>> +all_same (struct file_change_detection *array, size_t length)
>> +{
>> +  for (size_t i = 0; i < length; ++i)
>> +    for (size_t j = 0; j < length; ++j)
>> +      {
>> +        if (test_verbose > 0)
>> +          printf ("info: comparing %zu and %zu\n", i, j);
>> +        TEST_VERIFY (file_is_unchanged (array + i, array + j));
>> +      }
>> +}
>> +
>
> Wouldn't it the following be slight better?
>
>   for (size_t i = 0; i < length - 1; ++i)
>     for (size_t j = i + 1 < length, j++)
>        
> Or do you really to check all possible permutations?

Iterating over all pairs checks that the operation is commutative.

>> +  /* Wait for a file change.  Depending on file system time stamp
>> +     resolution, this subtest blocks for a while.  */
>> +  for (int use_stdio = 0; use_stdio < 2; ++use_stdio)
>> +    {
>> +      struct file_change_detection initial;
>> +      TEST_VERIFY (file_change_detection_for_path (&initial, path_file1));
>> +      while (true)
>> +        {
>> +          support_write_file_string (path_file1, "line\n");
>> +          struct file_change_detection current;
>> +          if (use_stdio)
>> +            TEST_VERIFY (file_change_detection_for_fp (&current, fp_file1));
>> +          else
>> +            TEST_VERIFY (file_change_detection_for_path (&current, path_file1));
>> +          if (!file_is_unchanged (&initial, &current))
>> +            break;
>> +          /* Wait for a bit to reduce system load.  */
>> +          usleep (100 * 1000);
>> +        }
>> +    }
>
> Ok, although the usleep seems excessive large (the testing will most likely
> timeout prior usleep return).

Hmm, I thought that this would wait 100 milliseconds?  So the loop
should exit in a second or two with low-resolution timestamps in the
file system.