* [Qemu-devel] [PATCH] Fix TAP networking on host kernels without IFF_VNET_HDR support @ 2009-11-24 9:06 Pierre Riteau 2009-11-24 10:05 ` [Qemu-devel] [PATCH] net: initialize vnet_hdr in net_tap_init() Mark McLoughlin 2009-11-24 10:28 ` [Qemu-devel] Re: [PATCH] Fix TAP networking on host kernels without IFF_VNET_HDR support Mark McLoughlin 0 siblings, 2 replies; 8+ messages in thread From: Pierre Riteau @ 2009-11-24 9:06 UTC (permalink / raw) To: qemu-devel, Mark McLoughlin; +Cc: Pierre Riteau vnet_hdr is initialized at 1 by default. We need to reset it to 0 if the kernel doesn't support IFF_VNET_HDR. Signed-off-by: Pierre Riteau <Pierre.Riteau@irisa.fr> --- net/tap-linux.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/net/tap-linux.c b/net/tap-linux.c index 0f621a2..e4f7e27 100644 --- a/net/tap-linux.c +++ b/net/tap-linux.c @@ -52,6 +52,8 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr, int vnet_hdr_required features & IFF_VNET_HDR) { *vnet_hdr = 1; ifr.ifr_flags |= IFF_VNET_HDR; + } else { + *vnet_hdr = 0; } if (vnet_hdr_required && !*vnet_hdr) { -- 1.6.5 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH] net: initialize vnet_hdr in net_tap_init() 2009-11-24 9:06 [Qemu-devel] [PATCH] Fix TAP networking on host kernels without IFF_VNET_HDR support Pierre Riteau @ 2009-11-24 10:05 ` Mark McLoughlin 2009-11-24 10:24 ` Pierre Riteau 2009-11-24 10:28 ` [Qemu-devel] Re: [PATCH] Fix TAP networking on host kernels without IFF_VNET_HDR support Mark McLoughlin 1 sibling, 1 reply; 8+ messages in thread From: Mark McLoughlin @ 2009-11-24 10:05 UTC (permalink / raw) To: Pierre Riteau; +Cc: qemu-devel Hi Pierre, On Tue, 2009-11-24 at 10:06 +0100, Pierre Riteau wrote: > vnet_hdr is initialized at 1 by default. We need to reset it to 0 if > the kernel doesn't support IFF_VNET_HDR. Thanks for the patch, but I'd prefer us to make sure we catch all cases. Does this work for you? Thanks, Mark. From: Mark McLoughlin <markmc@redhat.com> Subject: [PATCH] net: initialize vnet_hdr in net_tap_init() Don't assume that all tap_open() implementations will set it to zero if VNET_HDR support isn't found. Fixes tap networking on host kernels lacking IFF_VNET_HDR support. Reported-by: Pierre Riteau <Pierre.Riteau@irisa.fr> Signed-off-by: Mark McLoughlin <markmc@redhat.com> --- net/tap.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/net/tap.c b/net/tap.c index d34feec..7fb9e16 100644 --- a/net/tap.c +++ b/net/tap.c @@ -378,7 +378,7 @@ static int net_tap_init(QemuOpts *opts, int *vnet_hdr) int net_init_tap(QemuOpts *opts, Monitor *mon, const char *name, VLANState *vlan) { TAPState *s; - int fd, vnet_hdr; + int fd, vnet_hdr = 0; if (qemu_opt_get(opts, "fd")) { if (qemu_opt_get(opts, "ifname") || -- 1.6.5.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] net: initialize vnet_hdr in net_tap_init() 2009-11-24 10:05 ` [Qemu-devel] [PATCH] net: initialize vnet_hdr in net_tap_init() Mark McLoughlin @ 2009-11-24 10:24 ` Pierre Riteau 0 siblings, 0 replies; 8+ messages in thread From: Pierre Riteau @ 2009-11-24 10:24 UTC (permalink / raw) To: Mark McLoughlin; +Cc: qemu-devel On 24 nov. 2009, at 11:05, Mark McLoughlin wrote: > Hi Pierre, > > On Tue, 2009-11-24 at 10:06 +0100, Pierre Riteau wrote: >> vnet_hdr is initialized at 1 by default. We need to reset it to 0 if >> the kernel doesn't support IFF_VNET_HDR. > > Thanks for the patch, but I'd prefer us to make sure we catch all cases. > > Does this work for you? > > Thanks, > Mark. No it doesn't fix the problem, since vnet_hdr is then changed to 1 in net_tap_init by this line, just before calling tap_open: *vnet_hdr = qemu_opt_get_bool(opts, "vnet_hdr", 1); I run qemu like this: sudo qemu -m 1024 -hda /mnt/hda.img -net nic,macaddr=DE:AD:BE:EF:69:25 -net tap,script=/home/priteau/qemu-ifup -boot c -vnc 0:0 -- Pierre Riteau -- http://perso.univ-rennes1.fr/pierre.riteau/ ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] Re: [PATCH] Fix TAP networking on host kernels without IFF_VNET_HDR support 2009-11-24 9:06 [Qemu-devel] [PATCH] Fix TAP networking on host kernels without IFF_VNET_HDR support Pierre Riteau 2009-11-24 10:05 ` [Qemu-devel] [PATCH] net: initialize vnet_hdr in net_tap_init() Mark McLoughlin @ 2009-11-24 10:28 ` Mark McLoughlin 2009-11-24 11:17 ` Pierre Riteau 1 sibling, 1 reply; 8+ messages in thread From: Mark McLoughlin @ 2009-11-24 10:28 UTC (permalink / raw) To: Pierre Riteau; +Cc: qemu-devel On Tue, 2009-11-24 at 10:06 +0100, Pierre Riteau wrote: > vnet_hdr is initialized at 1 by default. We need to reset it to 0 if > the kernel doesn't support IFF_VNET_HDR. > > Signed-off-by: Pierre Riteau <Pierre.Riteau@irisa.fr> Thanks Pierre, I see why this is needed now Acked-by: Mark McLoughlin <markmc@redhat.com> Cheers, Mark. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] Re: [PATCH] Fix TAP networking on host kernels without IFF_VNET_HDR support 2009-11-24 10:28 ` [Qemu-devel] Re: [PATCH] Fix TAP networking on host kernels without IFF_VNET_HDR support Mark McLoughlin @ 2009-11-24 11:17 ` Pierre Riteau 2009-11-24 11:22 ` Mark McLoughlin 0 siblings, 1 reply; 8+ messages in thread From: Pierre Riteau @ 2009-11-24 11:17 UTC (permalink / raw) To: Mark McLoughlin; +Cc: qemu-devel On 24 nov. 2009, at 11:28, Mark McLoughlin wrote: > On Tue, 2009-11-24 at 10:06 +0100, Pierre Riteau wrote: >> vnet_hdr is initialized at 1 by default. We need to reset it to 0 if >> the kernel doesn't support IFF_VNET_HDR. >> >> Signed-off-by: Pierre Riteau <Pierre.Riteau@irisa.fr> > > Thanks Pierre, I see why this is needed now > > Acked-by: Mark McLoughlin <markmc@redhat.com> > > Cheers, > Mark. Thanks for your rapid answer! BTW, every time I run qemu I see this error message: TUNSETOFFLOAD ioctl() failed: Invalid argument It is caused by the piece of code at the end of net/tap-linux.c: if (ioctl(fd, TUNSETOFFLOAD, offload) != 0) { offload &= ~TUN_F_UFO; if (ioctl(fd, TUNSETOFFLOAD, offload) != 0) { fprintf(stderr, "TUNSETOFFLOAD ioctl() failed: %s\n", strerror(errno)); } } Isn't there a way to detect whether the kernel supports the TUNSETOFFLOAD ioctl at all? -- Pierre Riteau -- http://perso.univ-rennes1.fr/pierre.riteau/ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] Re: [PATCH] Fix TAP networking on host kernels without IFF_VNET_HDR support 2009-11-24 11:17 ` Pierre Riteau @ 2009-11-24 11:22 ` Mark McLoughlin 2009-11-24 21:27 ` Pierre Riteau 0 siblings, 1 reply; 8+ messages in thread From: Mark McLoughlin @ 2009-11-24 11:22 UTC (permalink / raw) To: Pierre Riteau; +Cc: qemu-devel On Tue, 2009-11-24 at 12:17 +0100, Pierre Riteau wrote: > On 24 nov. 2009, at 11:28, Mark McLoughlin wrote: > > > On Tue, 2009-11-24 at 10:06 +0100, Pierre Riteau wrote: > >> vnet_hdr is initialized at 1 by default. We need to reset it to 0 if > >> the kernel doesn't support IFF_VNET_HDR. > >> > >> Signed-off-by: Pierre Riteau <Pierre.Riteau@irisa.fr> > > > > Thanks Pierre, I see why this is needed now > > > > Acked-by: Mark McLoughlin <markmc@redhat.com> > > > > Cheers, > > Mark. > > > Thanks for your rapid answer! > > BTW, every time I run qemu I see this error message: > > TUNSETOFFLOAD ioctl() failed: Invalid argument > > It is caused by the piece of code at the end of net/tap-linux.c: > > if (ioctl(fd, TUNSETOFFLOAD, offload) != 0) { > offload &= ~TUN_F_UFO; > if (ioctl(fd, TUNSETOFFLOAD, offload) != 0) { > fprintf(stderr, "TUNSETOFFLOAD ioctl() failed: %s\n", > strerror(errno)); > } > } > > Isn't there a way to detect whether the kernel supports the > TUNSETOFFLOAD ioctl at all? The kernel will set errno to EINVAL if TUNSETOFFLOAD isn't supported, so we could just ignore that case: if (ioctl(fd, TUNSETOFFLOAD, offload) != 0) { offload &= ~TUN_F_UFO; if (ioctl(fd, TUNSETOFFLOAD, offload) != 0 && errno != EINVAL) { fprintf(stderr, "TUNSETOFFLOAD ioctl() failed: %s\n", strerror(errno)); } } The only concern is that we'll also miss out on an error message if EINVAL is set for another reason. Currently, the only other reason if we pass a offload flag not supported by the kernel, but that should never happen. Feel free to send a patch with that change and I'll ack it Thanks, Mark. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] Re: [PATCH] Fix TAP networking on host kernels without IFF_VNET_HDR support 2009-11-24 11:22 ` Mark McLoughlin @ 2009-11-24 21:27 ` Pierre Riteau 2009-11-25 8:55 ` Mark McLoughlin 0 siblings, 1 reply; 8+ messages in thread From: Pierre Riteau @ 2009-11-24 21:27 UTC (permalink / raw) To: Mark McLoughlin; +Cc: qemu-devel On 24 nov. 2009, at 12:22, Mark McLoughlin wrote: > On Tue, 2009-11-24 at 12:17 +0100, Pierre Riteau wrote: >> On 24 nov. 2009, at 11:28, Mark McLoughlin wrote: >> >>> On Tue, 2009-11-24 at 10:06 +0100, Pierre Riteau wrote: >>>> vnet_hdr is initialized at 1 by default. We need to reset it to 0 if >>>> the kernel doesn't support IFF_VNET_HDR. >>>> >>>> Signed-off-by: Pierre Riteau <Pierre.Riteau@irisa.fr> >>> >>> Thanks Pierre, I see why this is needed now >>> >>> Acked-by: Mark McLoughlin <markmc@redhat.com> >>> >>> Cheers, >>> Mark. >> >> >> Thanks for your rapid answer! >> >> BTW, every time I run qemu I see this error message: >> >> TUNSETOFFLOAD ioctl() failed: Invalid argument >> >> It is caused by the piece of code at the end of net/tap-linux.c: >> >> if (ioctl(fd, TUNSETOFFLOAD, offload) != 0) { >> offload &= ~TUN_F_UFO; >> if (ioctl(fd, TUNSETOFFLOAD, offload) != 0) { >> fprintf(stderr, "TUNSETOFFLOAD ioctl() failed: %s\n", >> strerror(errno)); >> } >> } >> >> Isn't there a way to detect whether the kernel supports the >> TUNSETOFFLOAD ioctl at all? > > The kernel will set errno to EINVAL if TUNSETOFFLOAD isn't supported, so > we could just ignore that case: > > if (ioctl(fd, TUNSETOFFLOAD, offload) != 0) { > offload &= ~TUN_F_UFO; > if (ioctl(fd, TUNSETOFFLOAD, offload) != 0 && errno != EINVAL) { > fprintf(stderr, "TUNSETOFFLOAD ioctl() failed: %s\n", > strerror(errno)); > } > } > > The only concern is that we'll also miss out on an error message if > EINVAL is set for another reason. Currently, the only other reason if we > pass a offload flag not supported by the kernel, but that should never > happen. > > Feel free to send a patch with that change and I'll ack it > > Thanks, > Mark. > Couldn't we probe the kernel with a 0 offload value to check if it supports TUNSETOFFLOAD? I tried the following and it works on my 2.6.26 Debian kernel. What do you think? I will send a proper patch if you agree. diff --git a/net/tap-linux.c b/net/tap-linux.c index 0f621a2..e038e1a 100644 --- a/net/tap-linux.c +++ b/net/tap-linux.c @@ -129,6 +129,11 @@ void tap_fd_set_offload(int fd, int csum, int tso4, { unsigned int offload = 0; + /* Check if our kernel supports TUNSETOFFLOAD */ + if (ioctl(fd, TUNSETOFFLOAD, 0) != 0 && errno == EINVAL) { + return; + } + if (csum) { offload |= TUN_F_CSUM; if (tso4) -- Pierre Riteau -- http://perso.univ-rennes1.fr/pierre.riteau/ ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] Re: [PATCH] Fix TAP networking on host kernels without IFF_VNET_HDR support 2009-11-24 21:27 ` Pierre Riteau @ 2009-11-25 8:55 ` Mark McLoughlin 0 siblings, 0 replies; 8+ messages in thread From: Mark McLoughlin @ 2009-11-25 8:55 UTC (permalink / raw) To: Pierre Riteau; +Cc: qemu-devel On Tue, 2009-11-24 at 22:27 +0100, Pierre Riteau wrote: > On 24 nov. 2009, at 12:22, Mark McLoughlin wrote: > > > On Tue, 2009-11-24 at 12:17 +0100, Pierre Riteau wrote: > >> Isn't there a way to detect whether the kernel supports the > >> TUNSETOFFLOAD ioctl at all? > > > > The kernel will set errno to EINVAL if TUNSETOFFLOAD isn't supported, so > > we could just ignore that case: > > > > if (ioctl(fd, TUNSETOFFLOAD, offload) != 0) { > > offload &= ~TUN_F_UFO; > > if (ioctl(fd, TUNSETOFFLOAD, offload) != 0 && errno != EINVAL) { > > fprintf(stderr, "TUNSETOFFLOAD ioctl() failed: %s\n", > > strerror(errno)); > > } > > } > > > > The only concern is that we'll also miss out on an error message if > > EINVAL is set for another reason. Currently, the only other reason if we > > pass a offload flag not supported by the kernel, but that should never > > happen. > > > > Feel free to send a patch with that change and I'll ack it .... > > Couldn't we probe the kernel with a 0 offload value to check if it supports TUNSETOFFLOAD? > I tried the following and it works on my 2.6.26 Debian kernel. > What do you think? I will send a proper patch if you agree. > > diff --git a/net/tap-linux.c b/net/tap-linux.c > index 0f621a2..e038e1a 100644 > --- a/net/tap-linux.c > +++ b/net/tap-linux.c > @@ -129,6 +129,11 @@ void tap_fd_set_offload(int fd, int csum, int tso4, > { > unsigned int offload = 0; > > + /* Check if our kernel supports TUNSETOFFLOAD */ > + if (ioctl(fd, TUNSETOFFLOAD, 0) != 0 && errno == EINVAL) { > + return; > + } > + It's not ideal because a) it's another syscall and b) you're briefly disabling any flags that may have been set previously ... but in our case, neither is a real concern and it's a nice, simple solution. So, sounds good to me :) Cheers, Mark. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2009-11-25 8:58 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-11-24 9:06 [Qemu-devel] [PATCH] Fix TAP networking on host kernels without IFF_VNET_HDR support Pierre Riteau 2009-11-24 10:05 ` [Qemu-devel] [PATCH] net: initialize vnet_hdr in net_tap_init() Mark McLoughlin 2009-11-24 10:24 ` Pierre Riteau 2009-11-24 10:28 ` [Qemu-devel] Re: [PATCH] Fix TAP networking on host kernels without IFF_VNET_HDR support Mark McLoughlin 2009-11-24 11:17 ` Pierre Riteau 2009-11-24 11:22 ` Mark McLoughlin 2009-11-24 21:27 ` Pierre Riteau 2009-11-25 8:55 ` Mark McLoughlin
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).