From: Asias He <asias@redhat.com>
To: Rusty Russell <rusty@rustcorp.com.au>
Cc: Stephen Rothwell <sfr@canb.auug.org.au>,
kvm@vger.kernel.org, "Michael S. Tsirkin" <mst@redhat.com>,
netdev@vger.kernel.org, Randy Dunlap <rdunlap@infradead.org>,
linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org,
target-devel@vger.kernel.org, linux-next@vger.kernel.org
Subject: Re: [PATCH] vhost-scsi: Depend on NET for memcpy_fromiovec
Date: Thu, 16 May 2013 12:35:44 +0800 [thread overview]
Message-ID: <20130516043544.GA32099@hj.localdomain> (raw)
In-Reply-To: <87d2sra97h.fsf@rustcorp.com.au>
On Thu, May 16, 2013 at 01:04:58PM +0930, Rusty Russell wrote:
> Asias He <asias@redhat.com> writes:
> > On Wed, May 15, 2013 at 02:47:53PM +0930, Rusty Russell wrote:
> >> Asias He <asias@redhat.com> writes:
> >> > scsi.c includes vhost.c which uses memcpy_fromiovec.
> >> >
> >> > This patch fixes this build failure.
> >> >
> >> > From Randy Dunlap:
> >> > '''
> >> > on x86_64:
> >> >
> >> > ERROR: "memcpy_fromiovec" [drivers/vhost/vhost_scsi.ko] undefined!
> >> >
> >> > It needs to depend on NET since net/core/ provides that function.
> >> > '''
> >>
> >> Proper fix please.
> >
> > --verbose please ;-)
> >
> > Making VHOST_SCSI depends on NET looks weird but this is because vhost
> > core depends on it. A bunch of patches are cleaning this up. Since MST
> > wanted do the vhost.ko split up in 3.11, plus your WIP vringh work, so I
> > wanted the fix for 3.10 as minimum as possible.
>
> If this isn't the only symbol causing the problem, then this should be
> mentioned in the changelog. If it is, it should be fixed: we have
> plenty of time for that.
>
> Either way, your commit message or the commit itself needs to justify
> it!
memcpy_fromiovec is the only one causing the problem.
>
> > Other users are using memcpy_fromiovec and friends outside net. It seems
> > a good idea to put it in a util library. e.g. crypto/algif_skcipher.c
> > which also depends on NET for it.
> >
> >> Though I can't see why you thought this was a good idea. Nonetheless, I
> >> shan't highlight why: I have far too much respect for your intellects
> >> and abilities.
> >>
> >> No, don't thank me!
> >
> > Interesting.
>
> Heh... I originally wrote an explanation, then found it a bit insulting:
> I knew I didn't need to tell you :)
;-)
> How's this?
Looks good and the commit log is more informative.
The
memcpy_toiovec
memcpy_toiovecend
memcpy_fromiovec
memcpy_fromiovecend
are all not net specific.
How about move them all to lib/ ?
Also need to make sure all the callers have uio.h included. e.g.
drivers/dma/iovlock.c
> From: Rusty Russell <rusty@rustcorp.com.au>
> Subject: Hoist memcpy_fromiovec into lib/
>
> ERROR: "memcpy_fromiovec" [drivers/vhost/vhost_scsi.ko] undefined!
>
> That function is only present with CONFIG_NET. Turns out that
> crypto/algif_skcipher.c also uses that outside net, but it actually
> needs sockets anyway.
>
> socket.h already include uio.h, so no callers need updating.
>
> Reported-by: Randy Dunlap <rdunlap@infradead.org>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>
> diff --git a/include/linux/socket.h b/include/linux/socket.h
> index 428c37a..7266775 100644
> --- a/include/linux/socket.h
> +++ b/include/linux/socket.h
> @@ -305,7 +305,6 @@ struct ucred {
>
> extern void cred_to_ucred(struct pid *pid, const struct cred *cred, struct ucred *ucred);
>
> -extern int memcpy_fromiovec(unsigned char *kdata, struct iovec *iov, int len);
> extern int memcpy_fromiovecend(unsigned char *kdata, const struct iovec *iov,
> int offset, int len);
> extern int csum_partial_copy_fromiovecend(unsigned char *kdata,
> diff --git a/include/linux/uio.h b/include/linux/uio.h
> index 629aaf5..21628d3 100644
> --- a/include/linux/uio.h
> +++ b/include/linux/uio.h
> @@ -35,4 +35,6 @@ static inline size_t iov_length(const struct iovec *iov, unsigned long nr_segs)
> }
>
> unsigned long iov_shorten(struct iovec *iov, unsigned long nr_segs, size_t to);
> +
> +int memcpy_fromiovec(unsigned char *kdata, struct iovec *iov, int len);
> #endif
> diff --git a/lib/Makefile b/lib/Makefile
> index e9c52e1..2377211 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -9,7 +9,7 @@ endif
>
> lib-y := ctype.o string.o vsprintf.o cmdline.o \
> rbtree.o radix-tree.o dump_stack.o timerqueue.o\
> - idr.o int_sqrt.o extable.o \
> + idr.o int_sqrt.o extable.o iovec.o \
> sha1.o md5.o irq_regs.o reciprocal_div.o argv_split.o \
> proportions.o flex_proportions.o prio_heap.o ratelimit.o show_mem.o \
> is_single_threaded.o plist.o decompress.o kobject_uevent.o \
> diff --git a/lib/iovec.c b/lib/iovec.c
> new file mode 100644
> index 0000000..632c5ea
> --- /dev/null
> +++ b/lib/iovec.c
> @@ -0,0 +1,29 @@
> +#include <linux/uaccess.h>
> +#include <linux/export.h>
> +#include <linux/uio.h>
> +
> +/*
> + * Copy iovec to kernel. Returns -EFAULT on error.
> + *
> + * Note: this modifies the original iovec.
> + */
> +
> +int memcpy_fromiovec(unsigned char *kdata, struct iovec *iov, int len)
> +{
> + while (len > 0) {
> + if (iov->iov_len) {
> + int copy = min_t(unsigned int, len, iov->iov_len);
> + if (copy_from_user(kdata, iov->iov_base, copy))
> + return -EFAULT;
> + len -= copy;
> + kdata += copy;
> + iov->iov_base += copy;
> + iov->iov_len -= copy;
> + }
> + iov++;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(memcpy_fromiovec);
> +
> diff --git a/net/core/iovec.c b/net/core/iovec.c
> index 7e7aeb0..d81257f 100644
> --- a/net/core/iovec.c
> +++ b/net/core/iovec.c
> @@ -125,31 +125,6 @@ int memcpy_toiovecend(const struct iovec *iov, unsigned char *kdata,
> EXPORT_SYMBOL(memcpy_toiovecend);
>
> /*
> - * Copy iovec to kernel. Returns -EFAULT on error.
> - *
> - * Note: this modifies the original iovec.
> - */
> -
> -int memcpy_fromiovec(unsigned char *kdata, struct iovec *iov, int len)
> -{
> - while (len > 0) {
> - if (iov->iov_len) {
> - int copy = min_t(unsigned int, len, iov->iov_len);
> - if (copy_from_user(kdata, iov->iov_base, copy))
> - return -EFAULT;
> - len -= copy;
> - kdata += copy;
> - iov->iov_base += copy;
> - iov->iov_len -= copy;
> - }
> - iov++;
> - }
> -
> - return 0;
> -}
> -EXPORT_SYMBOL(memcpy_fromiovec);
> -
> -/*
> * Copy iovec from kernel. Returns -EFAULT on error.
> */
>
--
Asias
next prev parent reply other threads:[~2013-05-16 4:35 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-05-14 4:17 linux-next: Tree for May 14 Stephen Rothwell
2013-05-14 19:16 ` linux-next: Tree for May 14 (vhost_scsi) Randy Dunlap
2013-05-14 23:55 ` Stephen Rothwell
2013-05-15 0:59 ` [PATCH] vhost-scsi: Depend on NET for memcpy_fromiovec Asias He
2013-05-15 3:10 ` Nicholas A. Bellinger
2013-05-15 3:44 ` Asias He
2013-05-15 5:17 ` Rusty Russell
2013-05-15 22:37 ` Nicholas A. Bellinger
2013-05-15 23:35 ` Rusty Russell
2013-05-16 2:16 ` Asias He
2013-05-16 3:10 ` David Miller
2013-05-16 6:46 ` Michael S. Tsirkin
2013-05-16 9:10 ` David Miller
2013-05-16 1:48 ` Asias He
2013-05-16 6:42 ` Michael S. Tsirkin
2013-05-16 2:08 ` Asias He
2013-05-16 3:34 ` Rusty Russell
2013-05-16 3:55 ` Joe Perches
2013-05-16 23:42 ` Rusty Russell
2013-05-17 4:42 ` Randy Dunlap
2013-05-23 7:30 ` Michael S. Tsirkin
2013-05-16 4:35 ` Asias He [this message]
2013-05-16 6:36 ` Michael S. Tsirkin
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=20130516043544.GA32099@hj.localdomain \
--to=asias@redhat.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-next@vger.kernel.org \
--cc=mst@redhat.com \
--cc=netdev@vger.kernel.org \
--cc=rdunlap@infradead.org \
--cc=rusty@rustcorp.com.au \
--cc=sfr@canb.auug.org.au \
--cc=target-devel@vger.kernel.org \
--cc=virtualization@lists.linux-foundation.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).