From: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
To: Gerd Hoffmann <kraxel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arch-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
aarcange-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org
Subject: Re: [PATCH v4 2/3] Add preadv and pwritev system calls.
Date: Thu, 18 Dec 2008 13:34:33 +0100 [thread overview]
Message-ID: <200812181334.33719.arnd@arndb.de> (raw)
In-Reply-To: <1229600542-11585-3-git-send-email-kraxel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Thursday 18 December 2008, Gerd Hoffmann wrote:
> This prototype has one problem though: On 32bit archs is the (64bit)
> offset argument unaligned, which the syscall ABI of several archs
> doesn't allow to do. At least s390 needs a wrapper in glibc to handle
> this. As we'll need a wrappers in glibc anyway I've decided to push
> problem to glibc entriely and use a syscall prototype which works
> without arch-specific wrappers inside the kernel: The offset argument
> is explicitly splitted into two 32bit values.
Obviously, the interface looks good to me now.
Please remember to add the function prototypes to
include/linux/syscalls.h.
> +asmlinkage ssize_t
> +compat_sys_preadv(unsigned long fd, const struct compat_iovec __user *vec,
> + unsigned long vlen, u32 pos_high, u32 pos_low)
> +{
> + loff_t pos = ((loff_t)pos_high << 32) | pos_low;
This is whitespace damaged, as are the other four functions in the
same place.
> + struct file *file;
> + ssize_t ret = -EBADF;
> +
> + if (pos < 0)
> + return -EINVAL;
> +
> + file = fget(fd);
> + if (!file)
> + return -EBADF;
Any reason for using fget() here, but fget_light() in sys_preadv?
> + if (!(file->f_mode & FMODE_READ))
> + goto out;
> +
> + ret = -EINVAL;
> + if (!file->f_op || (!file->f_op->aio_read && !file->f_op->read))
> + goto out;
Maybe this logic could get moved into a compat_readv() function,
similar to vfs_readv(). The advantage would be that we can make
the native and compat paths more similar.
Arnd <><
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
WARNING: multiple messages have this Message-ID (diff)
From: Arnd Bergmann <arnd@arndb.de>
To: Gerd Hoffmann <kraxel@redhat.com>
Cc: linux-kernel@vger.kernel.org, linux-arch@vger.kernel.org,
linux-api@vger.kernel.org, aarcange@redhat.com
Subject: Re: [PATCH v4 2/3] Add preadv and pwritev system calls.
Date: Thu, 18 Dec 2008 13:34:33 +0100 [thread overview]
Message-ID: <200812181334.33719.arnd@arndb.de> (raw)
Message-ID: <20081218123433.kLnd1_8xXEj41uMwYB_Nv94TMGNIa3OpZlPvR0PPKfY@z> (raw)
In-Reply-To: <1229600542-11585-3-git-send-email-kraxel@redhat.com>
On Thursday 18 December 2008, Gerd Hoffmann wrote:
> This prototype has one problem though: On 32bit archs is the (64bit)
> offset argument unaligned, which the syscall ABI of several archs
> doesn't allow to do. At least s390 needs a wrapper in glibc to handle
> this. As we'll need a wrappers in glibc anyway I've decided to push
> problem to glibc entriely and use a syscall prototype which works
> without arch-specific wrappers inside the kernel: The offset argument
> is explicitly splitted into two 32bit values.
Obviously, the interface looks good to me now.
Please remember to add the function prototypes to
include/linux/syscalls.h.
> +asmlinkage ssize_t
> +compat_sys_preadv(unsigned long fd, const struct compat_iovec __user *vec,
> + unsigned long vlen, u32 pos_high, u32 pos_low)
> +{
> + loff_t pos = ((loff_t)pos_high << 32) | pos_low;
This is whitespace damaged, as are the other four functions in the
same place.
> + struct file *file;
> + ssize_t ret = -EBADF;
> +
> + if (pos < 0)
> + return -EINVAL;
> +
> + file = fget(fd);
> + if (!file)
> + return -EBADF;
Any reason for using fget() here, but fget_light() in sys_preadv?
> + if (!(file->f_mode & FMODE_READ))
> + goto out;
> +
> + ret = -EINVAL;
> + if (!file->f_op || (!file->f_op->aio_read && !file->f_op->read))
> + goto out;
Maybe this logic could get moved into a compat_readv() function,
similar to vfs_readv(). The advantage would be that we can make
the native and compat paths more similar.
Arnd <><
next prev parent reply other threads:[~2008-12-18 12:34 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2008-12-18 11:42 [PATCH v4 0/3] System call design: preadv & pwritev Gerd Hoffmann
2008-12-18 11:42 ` Gerd Hoffmann
2008-12-18 11:42 ` [PATCH v4 1/3] Add missing accounting calls to compat_sys_{readv,writev} Gerd Hoffmann
2008-12-18 11:42 ` [PATCH v4 2/3] Add preadv and pwritev system calls Gerd Hoffmann
[not found] ` <1229600542-11585-3-git-send-email-kraxel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2008-12-18 12:34 ` Arnd Bergmann [this message]
2008-12-18 12:34 ` Arnd Bergmann
[not found] ` <200812181334.33719.arnd-r2nGTMty4D4@public.gmane.org>
2008-12-18 13:37 ` Gerd Hoffmann
2008-12-18 13:37 ` Gerd Hoffmann
2008-12-18 11:42 ` [PATCH v4 3/3] MIPS: Add preadv(2) and pwritev(2) syscalls Gerd Hoffmann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=200812181334.33719.arnd@arndb.de \
--to=arnd-r2ngtmty4d4@public.gmane.org \
--cc=aarcange-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org \
--cc=kraxel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org \
--cc=linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=linux-arch-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.