On 06/03/2013 06:48 PM, Laszlo Ersek wrote: > comments below > > On 06/03/13 11:04, Jason Wang wrote: >> This is because vhostfdname were passed as NULL to net_init_tap_one() when >> vhostfd were not specified, but net_init_tap_one() will still pass it to >> monitor_handle_fd_param() when tap->has_vhostfds is true. Since file >> descriptor >> (fd, vhostfd) and file descriptor set (fds, vhostfds) were not compatible, so >> this patch forbids passing them to tap in the same time. >> >> This solve the segfault when passing the command line like: >> ./qemu-system-x86_64 -netdev tap,fd=2,vhost=on,vhostfds=baz,id=xyz >> >> Cc: Paolo Bonzini <pbonz...@redhat.com> >> Cc: Stefan Hajnoczi <shajn...@redhat.com> >> Cc: Laszlo Ersek <ler...@redhat.com> >> Cc: qemu-sta...@nongnu.org >> Signed-off-by: Jason Wang <jasow...@redhat.com> >> --- >> net/tap.c | 10 ++++++---- >> 1 files changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/net/tap.c b/net/tap.c >> index e0b7a2a..477505f 100644 >> --- a/net/tap.c >> +++ b/net/tap.c >> @@ -698,9 +698,10 @@ int net_init_tap(const NetClientOptions *opts, const >> char *name, >> if (tap->has_fd) { >> if (tap->has_ifname || tap->has_script || tap->has_downscript || >> tap->has_vnet_hdr || tap->has_helper || tap->has_queues || >> - tap->has_fds) { >> + tap->has_fds || tap->has_vhostfds) { >> error_report("ifname=, script=, downscript=, vnet_hdr=, " >> - "helper=, queues=, and fds= are invalid with fd="); >> + "helper=, queues=, fds=, and vhostfds= " >> + "are invalid with fd="); >> return -1; >> } >> > This seems OK, since > > has_fd precludes has_fds > - optional: has_vhostfd - optional: has_vhostfds > >> @@ -725,9 +726,10 @@ int net_init_tap(const NetClientOptions *opts, const >> char *name, >> >> if (tap->has_ifname || tap->has_script || tap->has_downscript || >> tap->has_vnet_hdr || tap->has_helper || tap->has_queues || >> - tap->has_fd) { >> + tap->has_fd || tap->has_vhostfd) { >> error_report("ifname=, script=, downscript=, vnet_hdr=, " >> - "helper=, queues=, and fd= are invalid with fds="); >> + "helper=, queues=, fd=, and vhostfd= " >> + "are invalid with fds="); >> return -1; >> } >> >> > I think this us OK too (implementing the above exclusion from the other > side), but in the second hunk "tap->has_fd" can never be true actually. > I think relying on that in both the condition and the error message here > would simplify them.
Ture > > > Finally, I think the "has_helper", and the outermost "else" branch > should both be updated as well, for consistency with hunk #1 here. These > branches use "vhostfdname" too. In these branches, > - both "has_fd" and "has_fds" are false (should simplify check & errmsg), > - "has_vhostfds" should be rejected. Sure will send v2. Thanks > Thanks > Laszlo >