On 7/2/20 10:18 AM, Philippe Mathieu-Daudé wrote: > On 7/2/20 10:17 AM, Laurent Vivier wrote: >> On 02/07/2020 10:00, Philippe Mathieu-Daudé wrote: >>> On 7/1/20 9:39 PM, Laurent Vivier wrote: >>>> qemu_set_nonblock() checks that the file descriptor can be used and, if >>>> not, crashes QEMU. An assert() is used for that. The use of assert() is >>>> used to detect programming error and the coredump will allow to debug >>>> the problem. >>>> >>>> But in the case of the tap device, this assert() can be triggered by >>>> a misconfiguration by the user. At startup, it's not a real problem, but it >>>> can also happen during the hot-plug of a new device, and here it's a >>>> problem because we can crash a perfectly healthy system. >>>> >>>> For instance: >>>> # ip link add link virbr0 name macvtap0 type macvtap mode bridge >>>> # ip link set macvtap0 up >>>> # TAP=/dev/tap$(ip -o link show macvtap0 | cut -d: -f1) >>>> # qemu-system-x86_64 -machine q35 -device >>>> pcie-root-port,id=pcie-root-port-0 -monitor stdio 9<> $TAP >>>> (qemu) netdev_add type=tap,id=hostnet0,vhost=on,fd=9 >>>> (qemu) device_add >>>> driver=virtio-net-pci,netdev=hostnet0,id=net0,bus=pcie-root-port-0 >>>> (qemu) device_del net0 >>>> (qemu) netdev_del hostnet0 >>>> (qemu) netdev_add type=tap,id=hostnet1,vhost=on,fd=9 >>>> qemu-system-x86_64: .../util/oslib-posix.c:247: qemu_set_nonblock: >>>> Assertion `f != -1' failed. >>>> Aborted (core dumped) >>>> >>>> To avoid that, add a function, qemu_try_set_nonblock(), that allows to >>>> report the >>>> problem without crashing. >>>> >>>> Signed-off-by: Laurent Vivier <lviv...@redhat.com> >>>> --- >>>> include/qemu/sockets.h | 1 + >>>> net/tap.c | 16 +++++++++--- >>>> util/oslib-posix.c | 26 +++++++++++++------ >>>> util/oslib-win32.c | 57 ++++++++++++++++++++++++------------------ >>>> 4 files changed, 64 insertions(+), 36 deletions(-) >>>> >>>> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h >>>> index 57cd049d6edd..7d1f8135767d 100644 >>>> --- a/include/qemu/sockets.h >>>> +++ b/include/qemu/sockets.h >>>> @@ -18,6 +18,7 @@ int qemu_accept(int s, struct sockaddr *addr, socklen_t >>>> *addrlen); >>>> int socket_set_cork(int fd, int v); >>>> int socket_set_nodelay(int fd); >>>> void qemu_set_block(int fd); >>>> +int qemu_try_set_nonblock(int fd); >>>> void qemu_set_nonblock(int fd); >>>> int socket_set_fast_reuse(int fd); >>>> >>>> diff --git a/net/tap.c b/net/tap.c >>>> index 6207f61f84ab..fb04c9044ce2 100644 >>>> --- a/net/tap.c >>>> +++ b/net/tap.c >>>> @@ -766,6 +766,7 @@ int net_init_tap(const Netdev *netdev, const char >>>> *name, >>>> Error *err = NULL; >>>> const char *vhostfdname; >>>> char ifname[128]; >>>> + int ret = 0; >>> >>> No need to zero-initialize. >>> >>> Otherwise: >>> Reviewed-by: Philippe Mathieu-Daudé <phi...@redhat.com> >>> >> >> I would agree with you but gcc doesn't: >> >> net/tap.c:769:9: error: ‘ret’ may be used uninitialized in this function >> [-Werror=maybe-uninitialized] >> 769 | int ret; >> | ^~~ > > I tried to outsmart GCC by manual review :/
IOW my R-b stands: Reviewed-by: Philippe Mathieu-Daudé <phi...@redhat.com> > >> >> if nfds == 0, we don't enter in the loop and ret is never set. >> >> Thanks, >> Laurent >> >
