Re: [PATCH] Fix p{readv,writev}{64} consolidation implementation

[Mike Frysinger <vapier at gentoo dot org>]

On 14 Jun 2016 18:54, Adhemerval Zanella wrote:
> This patch fixes the p{readv,writev}{64} consolidation implementation
> from commits 4e77815 and af5fdf5.  Different from pread/pwrite
> implementation, preadv/pwritev implementation does not require
> __ALIGNMENT_ARG because kernel syscall prototypes define
> the high and low part of the off_t, if it is the case, directly
> (different from pread/pwrite where the architecture ABI for passing
> 64-bit values must be in consideration for passsing the arguments).

i had looked at that specifically but thought it ok because the old code
was using the alignment arg.  was the old code broken too ?

this is what the preadv code looked like:
-ssize_t
-__libc_preadv (int fd, const struct iovec *vector, int count, off_t offset)
-{
-  assert (sizeof (offset) == 4);
-  return SYSCALL_CANCEL (preadv, fd,
-                         vector, count, __ALIGNMENT_ARG
-                         __LONG_LONG_PAIR (offset >> 31, offset));
-}

although i guess this isn't too surprising as this code was in the
generic sysdeps dir which currently doesn't have as many users as
we wish it did :).

> +#define FAIL() \
> +  do { printf ("error: failure on line %d\n", __LINE__); return 1; } while (0)
> ...
> +  ret = pwritev (temp_fd, iov, 2, 0);
> +  if (ret == -1)
> +    FAIL ();

as a personal stance, never been a big fan of messages that just have a
line number.  when you get reports/logs, you end having to chase down the
exact same source tree as the reporter.

why can't we just assert() everywhere ?  we rely on that in tests already
and we wouldn't have to do any checking otherwise.
  ret = pwritev (temp_fd, iov, 2, 0);
  assert (ret == sizeof buf1 + sizeof buf2);
-mike

Attachment: signature.asc
Description: Digital signature