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

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

[Li Qiang]

Also add diagnostics info in 'qemu_set_cloexec' so that we can know
what happen when error occurs.

Signed-off-by: Li Qiang <liq3ea@gmail.com>
---
Change since v1: add diagnostics info
 
 util/oslib-posix.c | 37 ++++++++++++++++++++++++++++++++-----
 1 file changed, 32 insertions(+), 5 deletions(-)

diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index c1bee2a581..14cbef1e35 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -38,6 +38,7 @@
 #include <libgen.h>
 #include <sys/signal.h>
 #include "qemu/cutils.h"
+#include "qemu/error-report.h"
 
 #ifdef CONFIG_LINUX
 #include <sys/syscall.h>
@@ -233,14 +234,32 @@ void qemu_set_block(int fd)
 {
     int f;
     f = fcntl(fd, F_GETFL);
-    fcntl(fd, F_SETFL, f & ~O_NONBLOCK);
+    if (f < 0) {
+        error_report("Unable to get file status flag on fd %d: %s(errno=%d)",
+                    fd, strerror(errno), errno);
+        abort();
+    }
+    if (fcntl(fd, F_SETFL, f & ~O_NONBLOCK) < 0) {
+        error_report("Unable to set blocking flag on fd %d: %s(errno=%d)",
+                    fd, strerror(errno), errno);
+        abort();
+    }
 }
 
 void qemu_set_nonblock(int fd)
 {
     int f;
     f = fcntl(fd, F_GETFL);
-    fcntl(fd, F_SETFL, f | O_NONBLOCK);
+    if (f < 0) {
+        error_report("Unable to get file status flag on fd %d: %s(errno=%d)",
+                    fd, strerror(errno), errno);
+        abort();
+    }
+    if (fcntl(fd, F_SETFL, f | O_NONBLOCK) < 0) {
+        error_report("Unable to set nonblocking flag on fd %d: %s(errno=%d)",
+                    fd, strerror(errno), errno);
+        abort();
+    }
 }
 
 int socket_set_fast_reuse(int fd)
@@ -259,9 +278,17 @@ void qemu_set_cloexec(int fd)
 {
     int f;
     f = fcntl(fd, F_GETFD);
-    assert(f != -1);
-    f = fcntl(fd, F_SETFD, f | FD_CLOEXEC);
-    assert(f != -1);
+    if (f < 0) {
+        error_report("Unable to get fd flags on fd %d: %s(errno=%d)",
+                    fd, strerror(errno), errno);
+        abort();
+    }
+    if (fcntl(fd, F_SETFD, f | FD_CLOEXEC) < 0) {
+        error_report("Unable to set fd close-on-exec flag on fd %d:"
+                    "%s(errno=%d)",
+                    fd, strerror(errno), errno);
+        abort();
+    }
 }
 
 /*
-- 
2.11.0

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

[Peter Maydell]

On Thu, 13 Dec 2018 at 11:37, Li Qiang <liq3ea@gmail.com> wrote:
>
> Also add diagnostics info in 'qemu_set_cloexec' so that we can know
> what happen when error occurs.
>
> Signed-off-by: Li Qiang <liq3ea@gmail.com>
> ---
> Change since v1: add diagnostics info

We need to fix the assert in the test suite first...

thanks
-- PMM

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

[Daniel P. Berrangé]

On Thu, Dec 13, 2018 at 11:45:22AM +0000, Peter Maydell wrote:
> On Thu, 13 Dec 2018 at 11:37, Li Qiang <liq3ea@gmail.com> wrote:
> >
> > Also add diagnostics info in 'qemu_set_cloexec' so that we can know
> > what happen when error occurs.
> >
> > Signed-off-by: Li Qiang <liq3ea@gmail.com>
> > ---
> > Change since v1: add diagnostics info
> 
> We need to fix the assert in the test suite first...

Hopefully the improved diagnostics in this v2 will give us a better idea
of what the test suite failure is likely to be when patchew reports in....

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 :|

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

[Li Qiang]

Peter Maydell <peter.maydell@linaro.org> 于2018年12月13日周四 下午7:45写道：

> On Thu, 13 Dec 2018 at 11:37, Li Qiang <liq3ea@gmail.com> wrote:
> >
> > Also add diagnostics info in 'qemu_set_cloexec' so that we can know
> > what happen when error occurs.
> >
> > Signed-off-by: Li Qiang <liq3ea@gmail.com>
> > ---
> > Change since v1: add diagnostics info
>
> We need to fix the assert in the test suite first...
>

Agree, as I  mentioned in the last email, this assert seems occurs in
aarch64.
But I have no this environment. So this patch add diagnostics info to let
the bot tell us
what happen.

Thanks,
Li Qiang



> thanks
> -- PMM
>

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

[Markus Armbruster]

Li Qiang <liq3ea@gmail.com> writes:

> Also add diagnostics info in 'qemu_set_cloexec' so that we can know
> what happen when error occurs.
>
> Signed-off-by: Li Qiang <liq3ea@gmail.com>
> ---
> Change since v1: add diagnostics info
>  
>  util/oslib-posix.c | 37 ++++++++++++++++++++++++++++++++-----
>  1 file changed, 32 insertions(+), 5 deletions(-)
>
> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> index c1bee2a581..14cbef1e35 100644
> --- a/util/oslib-posix.c
> +++ b/util/oslib-posix.c
> @@ -38,6 +38,7 @@
>  #include <libgen.h>
>  #include <sys/signal.h>
>  #include "qemu/cutils.h"
> +#include "qemu/error-report.h"
>  
>  #ifdef CONFIG_LINUX
>  #include <sys/syscall.h>
> @@ -233,14 +234,32 @@ void qemu_set_block(int fd)
>  {
>      int f;
>      f = fcntl(fd, F_GETFL);
> -    fcntl(fd, F_SETFL, f & ~O_NONBLOCK);
> +    if (f < 0) {
> +        error_report("Unable to get file status flag on fd %d: %s(errno=%d)",
> +                    fd, strerror(errno), errno);
> +        abort();
> +    }
> +    if (fcntl(fd, F_SETFL, f & ~O_NONBLOCK) < 0) {
> +        error_report("Unable to set blocking flag on fd %d: %s(errno=%d)",
> +                    fd, strerror(errno), errno);
> +        abort();
> +    }
>  }

If this is a programming error, i.e. one that can happen only when the
function gets misused, then abort() is appropriate, but error_report()
is putting lipstick on the pig.

If it's not a programming error, but something outside QEMU misbehaves
(user error, network outage, ...), then error_report() is appropriate,
but abort() is not.  exit(1) then.

[...]

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

[no-reply]

Patchew URL: https://patchew.org/QEMU/1544701071-2922-1-git-send-email-liq3ea@gmail.com/



Hi,

This series failed the docker-quick@centos7 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 docker-test-quick@centos7 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/1544701071-2922-1-git-send-email-liq3ea@gmail.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

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

[Li Qiang]

Hi all,

Here is the error.

  GTESTER check-qtest-x86_64
Unable to get file status flag on fd 21860: Bad file descriptor(errno=9)
  GTESTER check-qtest-aarch64
Broken pipe
GTester: last random seed: R02S3f0d6981dd97231d06e0b2966baf94b9
Unable to get file status flag on fd 21965: Bad file descriptor(errno=9)
Broken pipe
GTester: last random seed: R02S29fde958e7ee4c26c4f295ff4dbd47d4
Unable to get file status flag on fd 21890: Bad file descriptor(errno=9)
Broken pipe
GTester: last random seed: R02S6d074187e5c8501255c96b247f5c8e3f
Unable to get file status flag on fd 21923: Bad file descriptor(errno=9)
Broken pipe
GTester: last random seed: R02S446127f38eb9e8b4f181e6fc95026ba0
  GTESTER tests/test-qht-par
Could not access KVM kernel module: No such file or directory


The fd '21860' '21965', '21890', '21923' is a little strange. Following is
my guess.

21280 --> 0x5564
21965 --> 0x55CD
21890 --> 0x5582
21923 --> 0x55A3

Seems they are stack uninitialized value which 'fd's memory holds.
Seems 'qemu_chr_fe_get_msgfds' first failed, then the 'fd' is an
uninitialized value
cause my first patch 'assert' fails.

Thanks,
Li Qiang



<no-reply@patchew.org> 于2018年12月14日周五 上午12:01写道：

> Patchew URL:
> https://patchew.org/QEMU/1544701071-2922-1-git-send-email-liq3ea@gmail.com/
>
>
>
> Hi,
>
> This series failed the docker-quick@centos7 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 docker-test-quick@centos7 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/1544701071-2922-1-git-send-email-liq3ea@gmail.com/testing.docker-quick@centos7/?type=message
> .
> ---
> Email generated automatically by Patchew [http://patchew.org/].
> Please send your feedback to patchew-devel@redhat.com

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

[Li Qiang]

Li Qiang <liq3ea@gmail.com> 于2018年12月14日周五 上午9:46写道：

> Hi all,
>
> Here is the error.
>
>   GTESTER check-qtest-x86_64
> Unable to get file status flag on fd 21860: Bad file descriptor(errno=9)
>   GTESTER check-qtest-aarch64
> Broken pipe
> GTester: last random seed: R02S3f0d6981dd97231d06e0b2966baf94b9
> Unable to get file status flag on fd 21965: Bad file descriptor(errno=9)
> Broken pipe
> GTester: last random seed: R02S29fde958e7ee4c26c4f295ff4dbd47d4
> Unable to get file status flag on fd 21890: Bad file descriptor(errno=9)
> Broken pipe
> GTester: last random seed: R02S6d074187e5c8501255c96b247f5c8e3f
> Unable to get file status flag on fd 21923: Bad file descriptor(errno=9)
> Broken pipe
> GTester: last random seed: R02S446127f38eb9e8b4f181e6fc95026ba0
>   GTESTER tests/test-qht-par
> Could not access KVM kernel module: No such file or directory
>
>
> The fd '21860' '21965', '21890', '21923' is a little strange. Following is
> my guess.
>
> 21280 --> 0x5564
> 21965 --> 0x55CD
> 21890 --> 0x5582
> 21923 --> 0x55A3
>
> Seems they are stack uninitialized value which 'fd's memory holds.
> Seems 'qemu_chr_fe_get_msgfds' first failed, then the 'fd' is an
> uninitialized value
> cause my first patch 'assert' fails.
>
>

First of all I want to know does the following error means?
doesn't it mean "the x86 qtest is ok and aarch64 is not ok"?


  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


This issue is caused by 'chr_read' in vhost-user-test.c processes
'VHOST_USER_SET_VRING_CALL'.
qemu_chr_fe_get_msgfds->tcp_get_msgfds

static int tcp_get_msgfds(Chardev *chr, int *fds, int num)
{
    SocketChardev *s = SOCKET_CHARDEV(chr);

    int to_copy = (s->read_msgfds_num < num) ? s->read_msgfds_num : num;

    assert(num <= TCP_MAX_FDS);

    if (to_copy) {
        int i;

        memcpy(fds, s->read_msgfds, to_copy * sizeof(int));

        /* Close unused fds */
        for (i = to_copy; i < s->read_msgfds_num; i++) {
            close(s->read_msgfds[i]);
        }

        g_free(s->read_msgfds);
        s->read_msgfds = 0;
        s->read_msgfds_num = 0;
    }

    return to_copy;
}

The 's->read_msgfds_num' is 0, so qemu_chr_fe_get_msgfds returns 0, and
'fd' be an uninitialized stack value.

The reason why 's->read_msgfds_num' is 0 is that in 'vhost_set_vring_file'.
the 'ioeventfd_enabled()' returns 0 and 'kvm_eventfds_allowed' is 0 which
means the
kernel doesn't support eventfd.

static int vhost_set_vring_file(struct vhost_dev *dev,
                                VhostUserRequest request,
                                struct vhost_vring_file *file)
{

    if (ioeventfd_enabled() && file->fd > 0) {
        fds[fd_num++] = file->fd;
    } else {
        msg.payload.u64 |= VHOST_USER_VRING_NOFD_MASK;
    }
  ...
    return 0;
}



After I look at the kernel, the arm platform seems doesn't support eventfd.

So if my understanding is correct in the first, '"the x86 qtest is ok and
aarch64 is not ok".
This can be explainable. But if not, there maybe another issue.

Since this is related with vhost-user-test, Cc Jason and Michael .

Thanks,
Li Qiang



> Thanks,
> Li Qiang
>
>
>
> <no-reply@patchew.org> 于2018年12月14日周五 上午12:01写道：
>
>> Patchew URL:
>> https://patchew.org/QEMU/1544701071-2922-1-git-send-email-liq3ea@gmail.com/
>>
>>
>>
>> Hi,
>>
>> This series failed the docker-quick@centos7 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 docker-test-quick@centos7 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/1544701071-2922-1-git-send-email-liq3ea@gmail.com/testing.docker-quick@centos7/?type=message
>> .
>> ---
>> Email generated automatically by Patchew [http://patchew.org/].
>> Please send your feedback to patchew-devel@redhat.com
>
>

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

[Peter Maydell]

On Fri, 14 Dec 2018 at 09:16, Li Qiang <liq3ea@gmail.com> wrote:
> First of all I want to know does the following error means?
> doesn't it mean "the x86 qtest is ok and aarch64 is not ok"?
>
>
>   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

I think maybe the make output is confusing you here, because it's
running both qtests in parallel. We don't ever run the
vhost-user-test for arm guests: tests/Makefile adds it only
to check-qtest-i386 and check-qtest-x86_64. So I think all
the warnings/errors here are coming from the x86 test.

> the 'ioeventfd_enabled()' returns 0 and 'kvm_eventfds_allowed' is 0 which means the
> kernel doesn't support eventfd.

> After I look at the kernel, the arm platform seems doesn't support eventfd.
>
> So if my understanding is correct in the first, '"the x86 qtest is ok and aarch64 is not ok".
> This can be explainable. But if not, there maybe another issue.

It looks to me from the investigation you did that the
vhost-user-test is assuming that we are using KVM
and ioeventfds, and is misbehaving when that is not true.
We should fix the test so that either it works in that
configuration, or correctly skips doing the test if it
is not applicable in a different config.

thanks
-- PMM