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. 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. Thanks Laszlo