Re: [Qemu-devel] [PATCH] util: check the return value of fcntl in qemu_set_{block, noblock}

[Li Qiang]

Daniel P. Berrangé <address@hidden> 于2018年12月13日周四 下午6:17写道：

> On Thu, Dec 13, 2018 at 05:56:24PM +0800, Li Qiang wrote:
> > Peter Maydell <address@hidden> 于2018年12月13日周四 下午5:31写道：
> >
> > > On Thu, 13 Dec 2018 at 06:58, <address@hidden> wrote:
> > > >
> > > > Patchew URL:
> > >
> https://patchew.org/QEMU/address@hidden/
> > > >
> > > >
> > > >
> > > > Hi,
> > > >
> > > > This series failed the address@hidden build test. Please find
> the
> > > testing commands and
> > > > their output below. If you have Docker installed, you can probably
> > > reproduce it
> > > > locally.
> > > >
> > > > === TEST SCRIPT BEGIN ===
> > > > #!/bin/bash
> > > > time make address@hidden SHOW_ENV=1 J=8
> > > > === TEST SCRIPT END ===
> > > >
> > > > libpmem support   no
> > > > libudev           no
> > > >
> > > > WARNING: Use of SDL 1.2 is deprecated and will be removed in
> > > > WARNING: future releases. Please switch to using SDL 2.0
> > > >
> > > > NOTE: cross-compilers enabled:  'cc'
> > > >   GEN     x86_64-softmmu/config-devices.mak.tmp
> > > >
> > > >
> > > > The full log is available at
> > > >
> > >
> http://patchew.org/logs/address@hidden/address@hidden/?type=message
> > > .
> > >
> > > Patchew's attempt to limit the log to only the section with
> > > the errors/warnings seems to have misfired here -- it looks
> > > like it's picked the first bit of the log with a warning/error
> > > rather than extracting all of them, which in this case happens
> > > to be the harmless complaint that this build setup doesn't
> > > have SDL2 installed.
> > >
> > > The actual cause of the failure is much lower down:
> > >
> > >
> > Indeed.
> >
> >
> > >   GTESTER check-qtest-aarch64
> > > vhost-user-test: /tmp/qemu-test/src/util/oslib-posix.c:245:
> > > qemu_set_nonblock: Assertion `f != -1' failed.
> > >
> >
> > So here means the fcntl call returns '-1'.
> > Seems this test have some bugs?
>
> Not neccessarily. It means that some code is caller qemu_set_nonblock
> with a file descriptor for this which is not valid. You'll have to
> debug what caller is triggering this to understand why
>

>From the bot, seems the error occurs in aarch64 qtest. (I have checked for
x86_64, no this error).

  GTESTER check-qtest-x86_64
  GTESTER check-qtest-aarch64
vhost-user-test: /tmp/qemu-test/src/util/oslib-posix.c:245:
qemu_set_nonblock: Assertion `f != -1' failed.
Broken pipe
GTester: last random seed: R02S61a1e35369394d7efb0a0e96d8af615d
  GTESTER tests/test-qht-par
vhost-user-test: /tmp/qemu-test/src/util/oslib-posix.c:245:
qemu_set_nonblock: Assertion `f != -1' failed.
Broken pipe
GTester: last random seed: R02Sbf8c21ef5f216840e073ff3e487dedbc
vhost-user-test: /tmp/qemu-test/src/util/oslib-posix.c:245:
qemu_set_nonblock: Assertion `f != -1' failed.
Broken pipe
GTester: last random seed: R02Sfe8cd276fddf3a92891cf274bf88e888
vhost-user-test: /tmp/qemu-test/src/util/oslib-posix.c:245:
qemu_set_nonblock: Assertion `f != -1' failed.
Broken pipe
GTester: last random seed: R02S7487fc065e22541ba4c78c0db6c61d3c
Could not access KVM kernel module: No such file or directory




>
> It might be as simple as something passing in an FD == -1, and indeed

I fear this is quite likely as we've been ignoring errors from
> qemu_set_nonblock forever.


Maybe, but if so, we need fix this(check 'fd' before calling
'qemu_set_{block，nonblock, cloexec}').
I think check 'fd' should be done before calling
'qemu_set_{block，nonblock, cloexec}', the caller of these function should
be responsible for the valid of 'fd'.

I will try to add some diagnostics info in the revised patch so that the
bot can give a more detailed information.


Thanks,
Li Qiang


> IOW, your change may well break existing
> code that is in fact working just fine today.
>
>
> Regards,
> Daniel
> --
> |: https://berrange.com      -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-
> https://www.instagram.com/dberrange :|
>