[v2,1/3] Consolidate fallocate{64} implementations

  Hi Siddhesh,

Thanks for the review, comments below:

On 29/09/2016 21:51, Siddhesh Poyarekar wrote:
>> diff --git a/sysdeps/unix/sysv/linux/tst-fallocate-common.c b/sysdeps/unix/sysv/linux/tst-fallocate-common.c
>> new file mode 100644
>> index 0000000..5ef66be
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/tst-fallocate-common.c

>> +
>> +void
>> +do_prepare (void)
>> +{
>> +  temp_fd = create_temp_file ("tst-fallocate.", &temp_filename);
>> +  if (temp_fd == -1)
>> +    {
>> +      printf ("cannot create temporary file: %m\n");
>> +      exit (1);
>> +    }
>> +}
>> +
>> +#define FAIL(str) \
>> +  do { printf ("error: %s (line %d)\n", str, __LINE__); return 1; } while (0)
> 
> Suggest: Use statement expressions since we use it widely in glibc.  It
> might be a nice low hanging fruit project to fix all our tests to use
> FAIL defined like this.

Ack, I changed to a statement expression.

> 
>> +
>> +static int
>> +do_test_with_offset (off_t offset)
>> +{
>> +  int ret;
>> +  struct stat finfo;
>> +  char bwrite[1024] = { 0xf0 };
>> +  char bread[1024];
> 
> Define a macro BLOCK_SIZE to 1024 and use that everywhere.

Ack.

>> +
>> +  /* It tries to fallocate 1024 bytes from 'offset' and then write 1024 bytes.
>> +     After both operation rewind the file descriptor and read 1024 bytes
>> +     and check if both buffer have the same contents.  */
>> +  ret = fallocate (temp_fd, 0, offset, 1024);
>> +  if (ret == -1)
>> +    FAIL ("fallocate failed");
>> +
>> +  ret = fstat (temp_fd, &finfo);
>> +  if (ret == -1)
>> +    FAIL ("fstat failed");
>> +
>> +  if (finfo.st_size < (offset + 1024))
>> +    FAIL ("size of first fallocate less than expected (1024)");
> 
> Suggest: Print offset + 1024 instead of just 1024.

Ack.

>> diff --git a/sysdeps/unix/sysv/linux/tst-fallocate64.c b/sysdeps/unix/sysv/linux/tst-fallocate64.c
>> new file mode 100644
>> index 0000000..78bcb8c
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/tst-fallocate64.c

>> +
>> +static int
>> +do_test (void)
>> +{
>> +  ssize_t ret;
>> +
>> +  ret = do_test_with_offset (0);
>> +  if (ret == -1)
>> +    return 1;
> 
> This will fail since you return 1 on failure from do_test_with_offset,
> not -1.

Ack, I fixed it.

> 
>> +
>> +  off_t base_offset = UINT32_MAX + 2048LL;
> 
> This will overflow on 32-bit.

Not really since we explicit build the test with _FILE_OFFSET_BITS=64 so
off_t will be 64-bit regardless.

> 
>> +  ret = do_test_with_offset (base_offset);
>> +  if (ret == -1)
>> +    return 1;
> 
> Again do_test_with_offset returns 1, not -1.

Ack.

Below it is a version with the test fixed based on your comments:

--

[v2,1/3] Consolidate fallocate{64} implementations

Commit Message

Comments

Patch