| Message ID | 1474994958-6007-1-git-send-email-felipe@nutanix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, Your series failed automatic build test. Please find the testing commands and their output below. If you have docker installed, you can probably reproduce it locally. Type: series Message-id: 1474994958-6007-1-git-send-email-felipe@nutanix.com Subject: [Qemu-devel] [PATCH] io: Fix double shift usages on QIOChannel features === TEST SCRIPT BEGIN === #!/bin/bash set -e git submodule update --init dtc # Let docker tests dump environment info export SHOW_ENV=1 make J=8 docker-test-quick@centos6 make J=8 docker-test-mingw@fedora === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test' 7882f36 io: Fix double shift usages on QIOChannel features === OUTPUT BEGIN === Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc' Cloning into 'dtc'... fatal: unable to connect to git.qemu-project.org: git.qemu-project.org[0: 140.211.15.109]: errno=Connection refused fatal: clone of 'git://git.qemu-project.org/dtc.git' into submodule path 'dtc' failed === OUTPUT END === Test command exited with code: 128 --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@freelists.org
Hi ----- Original Message ----- > When QIOChannels were introduced in 666a3af9, the feature bits were > defined shifted. However, when using them, the code was shifting them > again. The incorrect use was consistent until 74b6ce43, where > QIO_CHANNEL_FEATURE_LISTEN was defined shifted but tested unshifted. > And by luck, QIO_CHANNEL_FEATURE_LISTEN == (1 << QIO_CHANNEL_FEATURE_LISTEN), so it works :) > This patch fixes all uses of QIOChannel features. They are defined > shifted and therefore set unshifted. It also makes all feature tests to > use the qio_channel_has_feature() function. Looks good, we may want to have it in -stable. > > Signed-off-by: Felipe Franciosi <felipe@nutanix.com> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > io/channel-socket.c | 11 ++++++----- > io/channel-tls.c | 4 ++-- > io/channel-websock.c | 4 ++-- > io/channel.c | 10 ++++------ > migration/qemu-file-channel.c | 3 +-- > qemu-char.c | 3 +-- > 6 files changed, 16 insertions(+), 19 deletions(-) > > diff --git a/io/channel-socket.c b/io/channel-socket.c > index 196a4f1..bb0a0a7 100644 > --- a/io/channel-socket.c > +++ b/io/channel-socket.c > @@ -55,7 +55,7 @@ qio_channel_socket_new(void) > sioc->fd = -1; > > ioc = QIO_CHANNEL(sioc); > - ioc->features |= (1 << QIO_CHANNEL_FEATURE_SHUTDOWN); > + ioc->features |= QIO_CHANNEL_FEATURE_SHUTDOWN; > > #ifdef WIN32 > ioc->event = CreateEvent(NULL, FALSE, FALSE, NULL); > @@ -107,12 +107,12 @@ qio_channel_socket_set_fd(QIOChannelSocket *sioc, > #ifndef WIN32 > if (sioc->localAddr.ss_family == AF_UNIX) { > QIOChannel *ioc = QIO_CHANNEL(sioc); > - ioc->features |= (1 << QIO_CHANNEL_FEATURE_FD_PASS); > + ioc->features |= QIO_CHANNEL_FEATURE_FD_PASS; > } > #endif /* WIN32 */ > if (getsockopt(fd, SOL_SOCKET, SO_ACCEPTCONN, &val, &len) == 0 && val) { > QIOChannel *ioc = QIO_CHANNEL(sioc); > - ioc->features |= (1 << QIO_CHANNEL_FEATURE_LISTEN); > + ioc->features |= QIO_CHANNEL_FEATURE_LISTEN; > } > > return 0; > @@ -380,7 +380,7 @@ qio_channel_socket_accept(QIOChannelSocket *ioc, > > #ifndef WIN32 > if (cioc->localAddr.ss_family == AF_UNIX) { > - QIO_CHANNEL(cioc)->features |= (1 << QIO_CHANNEL_FEATURE_FD_PASS); > + QIO_CHANNEL(cioc)->features |= QIO_CHANNEL_FEATURE_FD_PASS; > } > #endif /* WIN32 */ > > @@ -403,7 +403,8 @@ static void qio_channel_socket_finalize(Object *obj) > QIOChannelSocket *ioc = QIO_CHANNEL_SOCKET(obj); > > if (ioc->fd != -1) { > - if (QIO_CHANNEL(ioc)->features & QIO_CHANNEL_FEATURE_LISTEN) { > + if (qio_channel_has_feature(QIO_CHANNEL(ioc), > + QIO_CHANNEL_FEATURE_LISTEN)) { > Error *err = NULL; > > socket_listen_cleanup(ioc->fd, &err); > diff --git a/io/channel-tls.c b/io/channel-tls.c > index 9a8525c..d22996b 100644 > --- a/io/channel-tls.c > +++ b/io/channel-tls.c > @@ -111,8 +111,8 @@ qio_channel_tls_new_client(QIOChannel *master, > ioc = QIO_CHANNEL(tioc); > > tioc->master = master; > - if (master->features & (1 << QIO_CHANNEL_FEATURE_SHUTDOWN)) { > - ioc->features |= (1 << QIO_CHANNEL_FEATURE_SHUTDOWN); > + if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOWN)) { > + ioc->features |= QIO_CHANNEL_FEATURE_SHUTDOWN; > } > object_ref(OBJECT(master)); > > diff --git a/io/channel-websock.c b/io/channel-websock.c > index 533bd4b..9accd16 100644 > --- a/io/channel-websock.c > +++ b/io/channel-websock.c > @@ -497,8 +497,8 @@ qio_channel_websock_new_server(QIOChannel *master) > ioc = QIO_CHANNEL(wioc); > > wioc->master = master; > - if (master->features & (1 << QIO_CHANNEL_FEATURE_SHUTDOWN)) { > - ioc->features |= (1 << QIO_CHANNEL_FEATURE_SHUTDOWN); > + if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOWN)) { > + ioc->features |= QIO_CHANNEL_FEATURE_SHUTDOWN; > } > object_ref(OBJECT(master)); > > diff --git a/io/channel.c b/io/channel.c > index 923c465..ebe9f86 100644 > --- a/io/channel.c > +++ b/io/channel.c > @@ -23,13 +23,11 @@ > #include "qapi/error.h" > #include "qemu/coroutine.h" > > -bool qio_channel_has_feature(QIOChannel *ioc, > - QIOChannelFeature feature) > +bool qio_channel_has_feature(QIOChannel *ioc, QIOChannelFeature feature) > { > - return ioc->features & (1 << feature); > + return ioc->features & feature; > } > > - > ssize_t qio_channel_readv_full(QIOChannel *ioc, > const struct iovec *iov, > size_t niov, > @@ -40,7 +38,7 @@ ssize_t qio_channel_readv_full(QIOChannel *ioc, > QIOChannelClass *klass = QIO_CHANNEL_GET_CLASS(ioc); > > if ((fds || nfds) && > - !(ioc->features & (1 << QIO_CHANNEL_FEATURE_FD_PASS))) { > + !qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_FD_PASS)) { > error_setg_errno(errp, EINVAL, > "Channel does not support file descriptor > passing"); > return -1; > @@ -60,7 +58,7 @@ ssize_t qio_channel_writev_full(QIOChannel *ioc, > QIOChannelClass *klass = QIO_CHANNEL_GET_CLASS(ioc); > > if ((fds || nfds) && > - !(ioc->features & (1 << QIO_CHANNEL_FEATURE_FD_PASS))) { > + !qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_FD_PASS)) { > error_setg_errno(errp, EINVAL, > "Channel does not support file descriptor > passing"); > return -1; > diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c > index 45c13f1..a39abd5 100644 > --- a/migration/qemu-file-channel.c > +++ b/migration/qemu-file-channel.c > @@ -105,8 +105,7 @@ static int channel_shutdown(void *opaque, > { > QIOChannel *ioc = QIO_CHANNEL(opaque); > > - if (qio_channel_has_feature(ioc, > - QIO_CHANNEL_FEATURE_SHUTDOWN)) { > + if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_SHUTDOWN)) { > QIOChannelShutdown mode; > if (rd && wr) { > mode = QIO_CHANNEL_SHUTDOWN_BOTH; > diff --git a/qemu-char.c b/qemu-char.c > index 8826419..b825331 100644 > --- a/qemu-char.c > +++ b/qemu-char.c > @@ -2787,8 +2787,7 @@ static int tcp_set_msgfds(CharDriverState *chr, int > *fds, int num) > s->write_msgfds_num = 0; > > if (!s->connected || > - !qio_channel_has_feature(s->ioc, > - QIO_CHANNEL_FEATURE_FD_PASS)) { > + !qio_channel_has_feature(s->ioc, QIO_CHANNEL_FEATURE_FD_PASS)) { > return -1; > } > > -- > 1.7.1 > >
On Tue, Sep 27, 2016 at 09:49:18AM -0700, Felipe Franciosi wrote: > When QIOChannels were introduced in 666a3af9, the feature bits were > defined shifted. However, when using them, the code was shifting them > again. The incorrect use was consistent until 74b6ce43, where > QIO_CHANNEL_FEATURE_LISTEN was defined shifted but tested unshifted. I'm more inclined to actually change the header file, so that they are defined unshifted, and fix the single place that tests unshifted. They are defined in an enum, so it was kind of odd to use shifted values in the first place. > This patch fixes all uses of QIOChannel features. They are defined > shifted and therefore set unshifted. It also makes all feature tests to > use the qio_channel_has_feature() function. Switching to use of qio_channel_has_feature() is a useful, but independant fix, so should be a separate commit really. > > Signed-off-by: Felipe Franciosi <felipe@nutanix.com> > --- > io/channel-socket.c | 11 ++++++----- > io/channel-tls.c | 4 ++-- > io/channel-websock.c | 4 ++-- > io/channel.c | 10 ++++------ > migration/qemu-file-channel.c | 3 +-- > qemu-char.c | 3 +-- > 6 files changed, 16 insertions(+), 19 deletions(-) The test/test-io-channel-*.c unit tests should be updated to validate the correct handling. > > diff --git a/io/channel-socket.c b/io/channel-socket.c > index 196a4f1..bb0a0a7 100644 > --- a/io/channel-socket.c > +++ b/io/channel-socket.c > @@ -55,7 +55,7 @@ qio_channel_socket_new(void) > sioc->fd = -1; > > ioc = QIO_CHANNEL(sioc); > - ioc->features |= (1 << QIO_CHANNEL_FEATURE_SHUTDOWN); > + ioc->features |= QIO_CHANNEL_FEATURE_SHUTDOWN; > > #ifdef WIN32 > ioc->event = CreateEvent(NULL, FALSE, FALSE, NULL); > @@ -107,12 +107,12 @@ qio_channel_socket_set_fd(QIOChannelSocket *sioc, > #ifndef WIN32 > if (sioc->localAddr.ss_family == AF_UNIX) { > QIOChannel *ioc = QIO_CHANNEL(sioc); > - ioc->features |= (1 << QIO_CHANNEL_FEATURE_FD_PASS); > + ioc->features |= QIO_CHANNEL_FEATURE_FD_PASS; > } > #endif /* WIN32 */ > if (getsockopt(fd, SOL_SOCKET, SO_ACCEPTCONN, &val, &len) == 0 && val) { > QIOChannel *ioc = QIO_CHANNEL(sioc); > - ioc->features |= (1 << QIO_CHANNEL_FEATURE_LISTEN); > + ioc->features |= QIO_CHANNEL_FEATURE_LISTEN; > } > > return 0; > @@ -380,7 +380,7 @@ qio_channel_socket_accept(QIOChannelSocket *ioc, > > #ifndef WIN32 > if (cioc->localAddr.ss_family == AF_UNIX) { > - QIO_CHANNEL(cioc)->features |= (1 << QIO_CHANNEL_FEATURE_FD_PASS); > + QIO_CHANNEL(cioc)->features |= QIO_CHANNEL_FEATURE_FD_PASS; > } > #endif /* WIN32 */ > > @@ -403,7 +403,8 @@ static void qio_channel_socket_finalize(Object *obj) > QIOChannelSocket *ioc = QIO_CHANNEL_SOCKET(obj); > > if (ioc->fd != -1) { > - if (QIO_CHANNEL(ioc)->features & QIO_CHANNEL_FEATURE_LISTEN) { > + if (qio_channel_has_feature(QIO_CHANNEL(ioc), > + QIO_CHANNEL_FEATURE_LISTEN)) { > Error *err = NULL; > > socket_listen_cleanup(ioc->fd, &err); > diff --git a/io/channel-tls.c b/io/channel-tls.c > index 9a8525c..d22996b 100644 > --- a/io/channel-tls.c > +++ b/io/channel-tls.c > @@ -111,8 +111,8 @@ qio_channel_tls_new_client(QIOChannel *master, > ioc = QIO_CHANNEL(tioc); > > tioc->master = master; > - if (master->features & (1 << QIO_CHANNEL_FEATURE_SHUTDOWN)) { > - ioc->features |= (1 << QIO_CHANNEL_FEATURE_SHUTDOWN); > + if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOWN)) { > + ioc->features |= QIO_CHANNEL_FEATURE_SHUTDOWN; > } > object_ref(OBJECT(master)); > > diff --git a/io/channel-websock.c b/io/channel-websock.c > index 533bd4b..9accd16 100644 > --- a/io/channel-websock.c > +++ b/io/channel-websock.c > @@ -497,8 +497,8 @@ qio_channel_websock_new_server(QIOChannel *master) > ioc = QIO_CHANNEL(wioc); > > wioc->master = master; > - if (master->features & (1 << QIO_CHANNEL_FEATURE_SHUTDOWN)) { > - ioc->features |= (1 << QIO_CHANNEL_FEATURE_SHUTDOWN); > + if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOWN)) { > + ioc->features |= QIO_CHANNEL_FEATURE_SHUTDOWN; > } > object_ref(OBJECT(master)); > > diff --git a/io/channel.c b/io/channel.c > index 923c465..ebe9f86 100644 > --- a/io/channel.c > +++ b/io/channel.c > @@ -23,13 +23,11 @@ > #include "qapi/error.h" > #include "qemu/coroutine.h" > > -bool qio_channel_has_feature(QIOChannel *ioc, > - QIOChannelFeature feature) > +bool qio_channel_has_feature(QIOChannel *ioc, QIOChannelFeature feature) Don't mix in whitespace changes like this. > { > - return ioc->features & (1 << feature); > + return ioc->features & feature; > } This is logically wrong - 'feature' can now contain multiple bits, but this is returning true if any single one of them is present, rather than if all are present. IMHO this is an example of why we should define them unshifted. > > - > ssize_t qio_channel_readv_full(QIOChannel *ioc, > const struct iovec *iov, > size_t niov, > @@ -40,7 +38,7 @@ ssize_t qio_channel_readv_full(QIOChannel *ioc, > QIOChannelClass *klass = QIO_CHANNEL_GET_CLASS(ioc); > > if ((fds || nfds) && > - !(ioc->features & (1 << QIO_CHANNEL_FEATURE_FD_PASS))) { > + !qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_FD_PASS)) { > error_setg_errno(errp, EINVAL, > "Channel does not support file descriptor passing"); > return -1; > @@ -60,7 +58,7 @@ ssize_t qio_channel_writev_full(QIOChannel *ioc, > QIOChannelClass *klass = QIO_CHANNEL_GET_CLASS(ioc); > > if ((fds || nfds) && > - !(ioc->features & (1 << QIO_CHANNEL_FEATURE_FD_PASS))) { > + !qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_FD_PASS)) { > error_setg_errno(errp, EINVAL, > "Channel does not support file descriptor passing"); > return -1; > diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c > index 45c13f1..a39abd5 100644 > --- a/migration/qemu-file-channel.c > +++ b/migration/qemu-file-channel.c > @@ -105,8 +105,7 @@ static int channel_shutdown(void *opaque, > { > QIOChannel *ioc = QIO_CHANNEL(opaque); > > - if (qio_channel_has_feature(ioc, > - QIO_CHANNEL_FEATURE_SHUTDOWN)) { > + if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_SHUTDOWN)) { > QIOChannelShutdown mode; > if (rd && wr) { > mode = QIO_CHANNEL_SHUTDOWN_BOTH; That's an unrelated arbitrary whitespace change. Please don't mix that with other functional changes. > diff --git a/qemu-char.c b/qemu-char.c > index 8826419..b825331 100644 > --- a/qemu-char.c > +++ b/qemu-char.c > @@ -2787,8 +2787,7 @@ static int tcp_set_msgfds(CharDriverState *chr, int *fds, int num) > s->write_msgfds_num = 0; > > if (!s->connected || > - !qio_channel_has_feature(s->ioc, > - QIO_CHANNEL_FEATURE_FD_PASS)) { > + !qio_channel_has_feature(s->ioc, QIO_CHANNEL_FEATURE_FD_PASS)) { > return -1; > } Again, don't do whitespace changes like this. Regards, Daniel
Hi Daniel, > On 27 Sep 2016, at 18:23, Daniel P. Berrange <berrange@redhat.com> wrote: > > On Tue, Sep 27, 2016 at 09:49:18AM -0700, Felipe Franciosi wrote: >> When QIOChannels were introduced in 666a3af9, the feature bits were >> defined shifted. However, when using them, the code was shifting them >> again. The incorrect use was consistent until 74b6ce43, where >> QIO_CHANNEL_FEATURE_LISTEN was defined shifted but tested unshifted. > > I'm more inclined to actually change the header file, so that they > are defined unshifted, and fix the single place that tests > unshifted. They are defined in an enum, so it was kind of odd to > use shifted values in the first place. It's not uncommon to specify shifted features/flags on enums: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/nl80211.h#n2661 https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/linux/nvme.h#n322 https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/linux/blk-mq.h#n194 I actually prefer defining them shifted, as proposed in my patch. And perhaps adding a qio_channel_set_feature() to completely abstract their usage. But I don't have strong preferences towards this and can change it if you really want me to. > >> This patch fixes all uses of QIOChannel features. They are defined >> shifted and therefore set unshifted. It also makes all feature tests to >> use the qio_channel_has_feature() function. > > Switching to use of qio_channel_has_feature() is a useful, but > independant fix, so should be a separate commit really. Sure I can separate that in another patch. Should I also add a qio_channel_set_feature()? I think that's a good idea. > >> >> Signed-off-by: Felipe Franciosi <felipe@nutanix.com> >> --- >> io/channel-socket.c | 11 ++++++----- >> io/channel-tls.c | 4 ++-- >> io/channel-websock.c | 4 ++-- >> io/channel.c | 10 ++++------ >> migration/qemu-file-channel.c | 3 +-- >> qemu-char.c | 3 +-- >> 6 files changed, 16 insertions(+), 19 deletions(-) > > The test/test-io-channel-*.c unit tests should be updated to > validate the correct handling. Oh, good point. Totally missed that. > >> >> diff --git a/io/channel-socket.c b/io/channel-socket.c >> index 196a4f1..bb0a0a7 100644 >> --- a/io/channel-socket.c >> +++ b/io/channel-socket.c >> @@ -55,7 +55,7 @@ qio_channel_socket_new(void) >> sioc->fd = -1; >> >> ioc = QIO_CHANNEL(sioc); >> - ioc->features |= (1 << QIO_CHANNEL_FEATURE_SHUTDOWN); >> + ioc->features |= QIO_CHANNEL_FEATURE_SHUTDOWN; >> >> #ifdef WIN32 >> ioc->event = CreateEvent(NULL, FALSE, FALSE, NULL); >> @@ -107,12 +107,12 @@ qio_channel_socket_set_fd(QIOChannelSocket *sioc, >> #ifndef WIN32 >> if (sioc->localAddr.ss_family == AF_UNIX) { >> QIOChannel *ioc = QIO_CHANNEL(sioc); >> - ioc->features |= (1 << QIO_CHANNEL_FEATURE_FD_PASS); >> + ioc->features |= QIO_CHANNEL_FEATURE_FD_PASS; >> } >> #endif /* WIN32 */ >> if (getsockopt(fd, SOL_SOCKET, SO_ACCEPTCONN, &val, &len) == 0 && val) { >> QIOChannel *ioc = QIO_CHANNEL(sioc); >> - ioc->features |= (1 << QIO_CHANNEL_FEATURE_LISTEN); >> + ioc->features |= QIO_CHANNEL_FEATURE_LISTEN; >> } >> >> return 0; >> @@ -380,7 +380,7 @@ qio_channel_socket_accept(QIOChannelSocket *ioc, >> >> #ifndef WIN32 >> if (cioc->localAddr.ss_family == AF_UNIX) { >> - QIO_CHANNEL(cioc)->features |= (1 << QIO_CHANNEL_FEATURE_FD_PASS); >> + QIO_CHANNEL(cioc)->features |= QIO_CHANNEL_FEATURE_FD_PASS; >> } >> #endif /* WIN32 */ >> >> @@ -403,7 +403,8 @@ static void qio_channel_socket_finalize(Object *obj) >> QIOChannelSocket *ioc = QIO_CHANNEL_SOCKET(obj); >> >> if (ioc->fd != -1) { >> - if (QIO_CHANNEL(ioc)->features & QIO_CHANNEL_FEATURE_LISTEN) { >> + if (qio_channel_has_feature(QIO_CHANNEL(ioc), >> + QIO_CHANNEL_FEATURE_LISTEN)) { >> Error *err = NULL; >> >> socket_listen_cleanup(ioc->fd, &err); >> diff --git a/io/channel-tls.c b/io/channel-tls.c >> index 9a8525c..d22996b 100644 >> --- a/io/channel-tls.c >> +++ b/io/channel-tls.c >> @@ -111,8 +111,8 @@ qio_channel_tls_new_client(QIOChannel *master, >> ioc = QIO_CHANNEL(tioc); >> >> tioc->master = master; >> - if (master->features & (1 << QIO_CHANNEL_FEATURE_SHUTDOWN)) { >> - ioc->features |= (1 << QIO_CHANNEL_FEATURE_SHUTDOWN); >> + if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOWN)) { >> + ioc->features |= QIO_CHANNEL_FEATURE_SHUTDOWN; >> } >> object_ref(OBJECT(master)); >> >> diff --git a/io/channel-websock.c b/io/channel-websock.c >> index 533bd4b..9accd16 100644 >> --- a/io/channel-websock.c >> +++ b/io/channel-websock.c >> @@ -497,8 +497,8 @@ qio_channel_websock_new_server(QIOChannel *master) >> ioc = QIO_CHANNEL(wioc); >> >> wioc->master = master; >> - if (master->features & (1 << QIO_CHANNEL_FEATURE_SHUTDOWN)) { >> - ioc->features |= (1 << QIO_CHANNEL_FEATURE_SHUTDOWN); >> + if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOWN)) { >> + ioc->features |= QIO_CHANNEL_FEATURE_SHUTDOWN; >> } >> object_ref(OBJECT(master)); >> >> diff --git a/io/channel.c b/io/channel.c >> index 923c465..ebe9f86 100644 >> --- a/io/channel.c >> +++ b/io/channel.c >> @@ -23,13 +23,11 @@ >> #include "qapi/error.h" >> #include "qemu/coroutine.h" >> >> -bool qio_channel_has_feature(QIOChannel *ioc, >> - QIOChannelFeature feature) >> +bool qio_channel_has_feature(QIOChannel *ioc, QIOChannelFeature feature) > > Don't mix in whitespace changes like this. I think the line break was a leftover from further back. There's clearly no need for it here, but I can separate it in another commit, too. > >> { >> - return ioc->features & (1 << feature); >> + return ioc->features & feature; >> } > > This is logically wrong - 'feature' can now contain multiple > bits, but this is returning true if any single one of them > is present, rather than if all are present. IMHO this is an > example of why we should define them unshifted. This looks correct to me. It's only wrong if we change the definition to be unshifted, which I believe is still on the table. :) > >> >> - >> ssize_t qio_channel_readv_full(QIOChannel *ioc, >> const struct iovec *iov, >> size_t niov, >> @@ -40,7 +38,7 @@ ssize_t qio_channel_readv_full(QIOChannel *ioc, >> QIOChannelClass *klass = QIO_CHANNEL_GET_CLASS(ioc); >> >> if ((fds || nfds) && >> - !(ioc->features & (1 << QIO_CHANNEL_FEATURE_FD_PASS))) { >> + !qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_FD_PASS)) { >> error_setg_errno(errp, EINVAL, >> "Channel does not support file descriptor passing"); >> return -1; >> @@ -60,7 +58,7 @@ ssize_t qio_channel_writev_full(QIOChannel *ioc, >> QIOChannelClass *klass = QIO_CHANNEL_GET_CLASS(ioc); >> >> if ((fds || nfds) && >> - !(ioc->features & (1 << QIO_CHANNEL_FEATURE_FD_PASS))) { >> + !qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_FD_PASS)) { >> error_setg_errno(errp, EINVAL, >> "Channel does not support file descriptor passing"); >> return -1; >> diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c >> index 45c13f1..a39abd5 100644 >> --- a/migration/qemu-file-channel.c >> +++ b/migration/qemu-file-channel.c >> @@ -105,8 +105,7 @@ static int channel_shutdown(void *opaque, >> { >> QIOChannel *ioc = QIO_CHANNEL(opaque); >> >> - if (qio_channel_has_feature(ioc, >> - QIO_CHANNEL_FEATURE_SHUTDOWN)) { >> + if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_SHUTDOWN)) { >> QIOChannelShutdown mode; >> if (rd && wr) { >> mode = QIO_CHANNEL_SHUTDOWN_BOTH; > > That's an unrelated arbitrary whitespace change. Please don't mix that > with other functional changes. As above, I'll put those on a separate patch. > >> diff --git a/qemu-char.c b/qemu-char.c >> index 8826419..b825331 100644 >> --- a/qemu-char.c >> +++ b/qemu-char.c >> @@ -2787,8 +2787,7 @@ static int tcp_set_msgfds(CharDriverState *chr, int *fds, int num) >> s->write_msgfds_num = 0; >> >> if (!s->connected || >> - !qio_channel_has_feature(s->ioc, >> - QIO_CHANNEL_FEATURE_FD_PASS)) { >> + !qio_channel_has_feature(s->ioc, QIO_CHANNEL_FEATURE_FD_PASS)) { >> return -1; >> } > > Again, don't do whitespace changes like this. As above, I'll put those on a separate patch. Let me know what you think about the (un)shifted definition before I send a v2. Thanks, Felipe > > Regards, > Daniel > -- > |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| > |: http://libvirt.org -o- http://virt-manager.org :| > |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
On Tue, Sep 27, 2016 at 05:57:12PM +0000, Felipe Franciosi wrote: > Hi Daniel, > > > On 27 Sep 2016, at 18:23, Daniel P. Berrange <berrange@redhat.com> wrote: > > > > On Tue, Sep 27, 2016 at 09:49:18AM -0700, Felipe Franciosi wrote: > >> When QIOChannels were introduced in 666a3af9, the feature bits were > >> defined shifted. However, when using them, the code was shifting them > >> again. The incorrect use was consistent until 74b6ce43, where > >> QIO_CHANNEL_FEATURE_LISTEN was defined shifted but tested unshifted. > > > > I'm more inclined to actually change the header file, so that they > > are defined unshifted, and fix the single place that tests > > unshifted. They are defined in an enum, so it was kind of odd to > > use shifted values in the first place. > > It's not uncommon to specify shifted features/flags on enums: > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/nl80211.h#n2661 > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/linux/nvme.h#n322 > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/linux/blk-mq.h#n194 > I actually prefer defining them shifted, as proposed in my patch. > And perhaps adding a qio_channel_set_feature() to completely > abstract their usage. But I don't have strong preferences towards > this and can change it if you really want me to. I'd really prefer them to be defined unshifted, just using the enum default value assignment. > >> This patch fixes all uses of QIOChannel features. They are defined > >> shifted and therefore set unshifted. It also makes all feature tests to > >> use the qio_channel_has_feature() function. > > > > Switching to use of qio_channel_has_feature() is a useful, but > > independant fix, so should be a separate commit really. > > Sure I can separate that in another patch. Should I also > add a qio_channel_set_feature()? I think that's a good idea. Yep, a set feature helper sounds like a reasonable addition. > > > >> { > >> - return ioc->features & (1 << feature); > >> + return ioc->features & feature; > >> } > > > > This is logically wrong - 'feature' can now contain multiple > > bits, but this is returning true if any single one of them > > is present, rather than if all are present. IMHO this is an > > example of why we should define them unshifted. > > This looks correct to me. It's only wrong if we change > the definition to be unshifted, which I believe is still > on the table. :) You've got it reversed - with the definitions shifted you can do qio_channel_has_feature(ioc, A | B) and it'll return true, even if only A is set. So if we were to keep the definitions shifted, then you'd actually need to have - return ioc->features & (1 << feature); + (return ioc->features & feature) == feature; but as above, I'd prefer to just have it unshifted. Regards, Daniel
Heya, > On 27 Sep 2016, at 19:04, Daniel P. Berrange <berrange@redhat.com> wrote: > > On Tue, Sep 27, 2016 at 05:57:12PM +0000, Felipe Franciosi wrote: >> Hi Daniel, >> >>> On 27 Sep 2016, at 18:23, Daniel P. Berrange <berrange@redhat.com> wrote: >>> >>> On Tue, Sep 27, 2016 at 09:49:18AM -0700, Felipe Franciosi wrote: >>>> When QIOChannels were introduced in 666a3af9, the feature bits were >>>> defined shifted. However, when using them, the code was shifting them >>>> again. The incorrect use was consistent until 74b6ce43, where >>>> QIO_CHANNEL_FEATURE_LISTEN was defined shifted but tested unshifted. >>> >>> I'm more inclined to actually change the header file, so that they >>> are defined unshifted, and fix the single place that tests >>> unshifted. They are defined in an enum, so it was kind of odd to >>> use shifted values in the first place. >> >> It's not uncommon to specify shifted features/flags on enums: >> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/nl80211.h#n2661 >> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/linux/nvme.h#n322 >> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/linux/blk-mq.h#n194 > >> I actually prefer defining them shifted, as proposed in my patch. >> And perhaps adding a qio_channel_set_feature() to completely >> abstract their usage. But I don't have strong preferences towards >> this and can change it if you really want me to. > > I'd really prefer them to be defined unshifted, just using the > enum default value assignment. Ok I'll change it. > >>>> This patch fixes all uses of QIOChannel features. They are defined >>>> shifted and therefore set unshifted. It also makes all feature tests to >>>> use the qio_channel_has_feature() function. >>> >>> Switching to use of qio_channel_has_feature() is a useful, but >>> independant fix, so should be a separate commit really. >> >> Sure I can separate that in another patch. Should I also >> add a qio_channel_set_feature()? I think that's a good idea. > > Yep, a set feature helper sounds like a reasonable addition. Sure I'll add it. > >>> >>>> { >>>> - return ioc->features & (1 << feature); >>>> + return ioc->features & feature; >>>> } >>> >>> This is logically wrong - 'feature' can now contain multiple >>> bits, but this is returning true if any single one of them >>> is present, rather than if all are present. IMHO this is an >>> example of why we should define them unshifted. >> >> This looks correct to me. It's only wrong if we change >> the definition to be unshifted, which I believe is still >> on the table. :) > > You've got it reversed - with the definitions shifted > you can do > > qio_channel_has_feature(ioc, A | B) > > and it'll return true, even if only A is set. Then maybe I got it really wrong. If you write "has_feature(A or B)", I'd expect it to return true if you have features A or B. Not _only_ if you have both. For that, I'd call has_feature(A) && has_feature(B). Alternatively, I'd define a has_features(mask) which could take OR'd features (plural) and behave as you defined below. > So if we > were to keep the definitions shifted, then you'd actually > need to have > > - return ioc->features & (1 << feature); > + (return ioc->features & feature) == feature; > > but as above, I'd prefer to just have it unshifted. Sure. As I said, I haven't got a strong preference. Felipe > > > Regards, > Daniel > -- > |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| > |: http://libvirt.org -o- http://virt-manager.org :| > |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
> On 27 Sep 2016, at 18:23, Daniel P. Berrange <berrange@redhat.com> wrote: > > On Tue, Sep 27, 2016 at 09:49:18AM -0700, Felipe Franciosi wrote: >> >> Signed-off-by: Felipe Franciosi <felipe@nutanix.com> >> --- >> io/channel-socket.c | 11 ++++++----- >> io/channel-tls.c | 4 ++-- >> io/channel-websock.c | 4 ++-- >> io/channel.c | 10 ++++------ >> migration/qemu-file-channel.c | 3 +-- >> qemu-char.c | 3 +-- >> 6 files changed, 16 insertions(+), 19 deletions(-) > > The test/test-io-channel-*.c unit tests should be updated to > validate the correct handling. Hi Daniel, Actually it looks like all tests are using the has_feature() wrapper already, so I'm not sure I need to fix anything here. Please let me know if I missed something. Thanks, Felipe
On Tue, Sep 27, 2016 at 06:35:41PM +0000, Felipe Franciosi wrote: > > > On 27 Sep 2016, at 18:23, Daniel P. Berrange <berrange@redhat.com> wrote: > > > > On Tue, Sep 27, 2016 at 09:49:18AM -0700, Felipe Franciosi wrote: > >> > >> Signed-off-by: Felipe Franciosi <felipe@nutanix.com> > >> --- > >> io/channel-socket.c | 11 ++++++----- > >> io/channel-tls.c | 4 ++-- > >> io/channel-websock.c | 4 ++-- > >> io/channel.c | 10 ++++------ > >> migration/qemu-file-channel.c | 3 +-- > >> qemu-char.c | 3 +-- > >> 6 files changed, 16 insertions(+), 19 deletions(-) > > > > The test/test-io-channel-*.c unit tests should be updated to > > validate the correct handling. > > Hi Daniel, > > Actually it looks like all tests are using the has_feature() wrapper > already, so I'm not sure I need to fix anything here. Please let me > know if I missed something. If we had full test coverage of the feature flags, then we should have seen a test failure for this bug. So there must be something we're not exercising in the test suite. Regards, Daniel
Hi Marc, > On 27 Sep 2016, at 18:12, Marc-André Lureau <mlureau@redhat.com> wrote: > > Hi > > ----- Original Message ----- >> When QIOChannels were introduced in 666a3af9, the feature bits were >> defined shifted. However, when using them, the code was shifting them >> again. The incorrect use was consistent until 74b6ce43, where >> QIO_CHANNEL_FEATURE_LISTEN was defined shifted but tested unshifted. >> > > And by luck, QIO_CHANNEL_FEATURE_LISTEN == (1 << QIO_CHANNEL_FEATURE_LISTEN), so it works :) Can you elaborate on that? Maybe I'm missing something obvious, but I'm confused as to how they are equivalent. Thanks, Felipe > >> This patch fixes all uses of QIOChannel features. They are defined >> shifted and therefore set unshifted. It also makes all feature tests to >> use the qio_channel_has_feature() function. > > Looks good, we may want to have it in -stable. > >> >> Signed-off-by: Felipe Franciosi <felipe@nutanix.com> > > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > >> --- >> io/channel-socket.c | 11 ++++++----- >> io/channel-tls.c | 4 ++-- >> io/channel-websock.c | 4 ++-- >> io/channel.c | 10 ++++------ >> migration/qemu-file-channel.c | 3 +-- >> qemu-char.c | 3 +-- >> 6 files changed, 16 insertions(+), 19 deletions(-) >> >> diff --git a/io/channel-socket.c b/io/channel-socket.c >> index 196a4f1..bb0a0a7 100644 >> --- a/io/channel-socket.c >> +++ b/io/channel-socket.c >> @@ -55,7 +55,7 @@ qio_channel_socket_new(void) >> sioc->fd = -1; >> >> ioc = QIO_CHANNEL(sioc); >> - ioc->features |= (1 << QIO_CHANNEL_FEATURE_SHUTDOWN); >> + ioc->features |= QIO_CHANNEL_FEATURE_SHUTDOWN; >> >> #ifdef WIN32 >> ioc->event = CreateEvent(NULL, FALSE, FALSE, NULL); >> @@ -107,12 +107,12 @@ qio_channel_socket_set_fd(QIOChannelSocket *sioc, >> #ifndef WIN32 >> if (sioc->localAddr.ss_family == AF_UNIX) { >> QIOChannel *ioc = QIO_CHANNEL(sioc); >> - ioc->features |= (1 << QIO_CHANNEL_FEATURE_FD_PASS); >> + ioc->features |= QIO_CHANNEL_FEATURE_FD_PASS; >> } >> #endif /* WIN32 */ >> if (getsockopt(fd, SOL_SOCKET, SO_ACCEPTCONN, &val, &len) == 0 && val) { >> QIOChannel *ioc = QIO_CHANNEL(sioc); >> - ioc->features |= (1 << QIO_CHANNEL_FEATURE_LISTEN); >> + ioc->features |= QIO_CHANNEL_FEATURE_LISTEN; >> } >> >> return 0; >> @@ -380,7 +380,7 @@ qio_channel_socket_accept(QIOChannelSocket *ioc, >> >> #ifndef WIN32 >> if (cioc->localAddr.ss_family == AF_UNIX) { >> - QIO_CHANNEL(cioc)->features |= (1 << QIO_CHANNEL_FEATURE_FD_PASS); >> + QIO_CHANNEL(cioc)->features |= QIO_CHANNEL_FEATURE_FD_PASS; >> } >> #endif /* WIN32 */ >> >> @@ -403,7 +403,8 @@ static void qio_channel_socket_finalize(Object *obj) >> QIOChannelSocket *ioc = QIO_CHANNEL_SOCKET(obj); >> >> if (ioc->fd != -1) { >> - if (QIO_CHANNEL(ioc)->features & QIO_CHANNEL_FEATURE_LISTEN) { >> + if (qio_channel_has_feature(QIO_CHANNEL(ioc), >> + QIO_CHANNEL_FEATURE_LISTEN)) { >> Error *err = NULL; >> >> socket_listen_cleanup(ioc->fd, &err); >> diff --git a/io/channel-tls.c b/io/channel-tls.c >> index 9a8525c..d22996b 100644 >> --- a/io/channel-tls.c >> +++ b/io/channel-tls.c >> @@ -111,8 +111,8 @@ qio_channel_tls_new_client(QIOChannel *master, >> ioc = QIO_CHANNEL(tioc); >> >> tioc->master = master; >> - if (master->features & (1 << QIO_CHANNEL_FEATURE_SHUTDOWN)) { >> - ioc->features |= (1 << QIO_CHANNEL_FEATURE_SHUTDOWN); >> + if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOWN)) { >> + ioc->features |= QIO_CHANNEL_FEATURE_SHUTDOWN; >> } >> object_ref(OBJECT(master)); >> >> diff --git a/io/channel-websock.c b/io/channel-websock.c >> index 533bd4b..9accd16 100644 >> --- a/io/channel-websock.c >> +++ b/io/channel-websock.c >> @@ -497,8 +497,8 @@ qio_channel_websock_new_server(QIOChannel *master) >> ioc = QIO_CHANNEL(wioc); >> >> wioc->master = master; >> - if (master->features & (1 << QIO_CHANNEL_FEATURE_SHUTDOWN)) { >> - ioc->features |= (1 << QIO_CHANNEL_FEATURE_SHUTDOWN); >> + if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOWN)) { >> + ioc->features |= QIO_CHANNEL_FEATURE_SHUTDOWN; >> } >> object_ref(OBJECT(master)); >> >> diff --git a/io/channel.c b/io/channel.c >> index 923c465..ebe9f86 100644 >> --- a/io/channel.c >> +++ b/io/channel.c >> @@ -23,13 +23,11 @@ >> #include "qapi/error.h" >> #include "qemu/coroutine.h" >> >> -bool qio_channel_has_feature(QIOChannel *ioc, >> - QIOChannelFeature feature) >> +bool qio_channel_has_feature(QIOChannel *ioc, QIOChannelFeature feature) >> { >> - return ioc->features & (1 << feature); >> + return ioc->features & feature; >> } >> >> - >> ssize_t qio_channel_readv_full(QIOChannel *ioc, >> const struct iovec *iov, >> size_t niov, >> @@ -40,7 +38,7 @@ ssize_t qio_channel_readv_full(QIOChannel *ioc, >> QIOChannelClass *klass = QIO_CHANNEL_GET_CLASS(ioc); >> >> if ((fds || nfds) && >> - !(ioc->features & (1 << QIO_CHANNEL_FEATURE_FD_PASS))) { >> + !qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_FD_PASS)) { >> error_setg_errno(errp, EINVAL, >> "Channel does not support file descriptor >> passing"); >> return -1; >> @@ -60,7 +58,7 @@ ssize_t qio_channel_writev_full(QIOChannel *ioc, >> QIOChannelClass *klass = QIO_CHANNEL_GET_CLASS(ioc); >> >> if ((fds || nfds) && >> - !(ioc->features & (1 << QIO_CHANNEL_FEATURE_FD_PASS))) { >> + !qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_FD_PASS)) { >> error_setg_errno(errp, EINVAL, >> "Channel does not support file descriptor >> passing"); >> return -1; >> diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c >> index 45c13f1..a39abd5 100644 >> --- a/migration/qemu-file-channel.c >> +++ b/migration/qemu-file-channel.c >> @@ -105,8 +105,7 @@ static int channel_shutdown(void *opaque, >> { >> QIOChannel *ioc = QIO_CHANNEL(opaque); >> >> - if (qio_channel_has_feature(ioc, >> - QIO_CHANNEL_FEATURE_SHUTDOWN)) { >> + if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_SHUTDOWN)) { >> QIOChannelShutdown mode; >> if (rd && wr) { >> mode = QIO_CHANNEL_SHUTDOWN_BOTH; >> diff --git a/qemu-char.c b/qemu-char.c >> index 8826419..b825331 100644 >> --- a/qemu-char.c >> +++ b/qemu-char.c >> @@ -2787,8 +2787,7 @@ static int tcp_set_msgfds(CharDriverState *chr, int >> *fds, int num) >> s->write_msgfds_num = 0; >> >> if (!s->connected || >> - !qio_channel_has_feature(s->ioc, >> - QIO_CHANNEL_FEATURE_FD_PASS)) { >> + !qio_channel_has_feature(s->ioc, QIO_CHANNEL_FEATURE_FD_PASS)) { >> return -1; >> } >> >> -- >> 1.7.1 >> >>
Hi ----- Original Message ----- > Hi Marc, > > > On 27 Sep 2016, at 18:12, Marc-André Lureau <mlureau@redhat.com> wrote: > > > > Hi > > > > ----- Original Message ----- > >> When QIOChannels were introduced in 666a3af9, the feature bits were > >> defined shifted. However, when using them, the code was shifting them > >> again. The incorrect use was consistent until 74b6ce43, where > >> QIO_CHANNEL_FEATURE_LISTEN was defined shifted but tested unshifted. > >> > > > > And by luck, QIO_CHANNEL_FEATURE_LISTEN == (1 << > > QIO_CHANNEL_FEATURE_LISTEN), so it works :) Oops, they are not, QIO_CHANNEL_FEATURE_LISTEN == 4 (I read 2), then I wonder how could 74b6ce43e work.. > > Can you elaborate on that? Maybe I'm missing something obvious, but I'm > confused as to how they are equivalent. > > Thanks, > Felipe > > > > >> This patch fixes all uses of QIOChannel features. They are defined > >> shifted and therefore set unshifted. It also makes all feature tests to > >> use the qio_channel_has_feature() function. > > > > Looks good, we may want to have it in -stable. > > > >> > >> Signed-off-by: Felipe Franciosi <felipe@nutanix.com> > > > > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > > >> --- > >> io/channel-socket.c | 11 ++++++----- > >> io/channel-tls.c | 4 ++-- > >> io/channel-websock.c | 4 ++-- > >> io/channel.c | 10 ++++------ > >> migration/qemu-file-channel.c | 3 +-- > >> qemu-char.c | 3 +-- > >> 6 files changed, 16 insertions(+), 19 deletions(-) > >> > >> diff --git a/io/channel-socket.c b/io/channel-socket.c > >> index 196a4f1..bb0a0a7 100644 > >> --- a/io/channel-socket.c > >> +++ b/io/channel-socket.c > >> @@ -55,7 +55,7 @@ qio_channel_socket_new(void) > >> sioc->fd = -1; > >> > >> ioc = QIO_CHANNEL(sioc); > >> - ioc->features |= (1 << QIO_CHANNEL_FEATURE_SHUTDOWN); > >> + ioc->features |= QIO_CHANNEL_FEATURE_SHUTDOWN; > >> > >> #ifdef WIN32 > >> ioc->event = CreateEvent(NULL, FALSE, FALSE, NULL); > >> @@ -107,12 +107,12 @@ qio_channel_socket_set_fd(QIOChannelSocket *sioc, > >> #ifndef WIN32 > >> if (sioc->localAddr.ss_family == AF_UNIX) { > >> QIOChannel *ioc = QIO_CHANNEL(sioc); > >> - ioc->features |= (1 << QIO_CHANNEL_FEATURE_FD_PASS); > >> + ioc->features |= QIO_CHANNEL_FEATURE_FD_PASS; > >> } > >> #endif /* WIN32 */ > >> if (getsockopt(fd, SOL_SOCKET, SO_ACCEPTCONN, &val, &len) == 0 && val) > >> { > >> QIOChannel *ioc = QIO_CHANNEL(sioc); > >> - ioc->features |= (1 << QIO_CHANNEL_FEATURE_LISTEN); > >> + ioc->features |= QIO_CHANNEL_FEATURE_LISTEN; > >> } > >> > >> return 0; > >> @@ -380,7 +380,7 @@ qio_channel_socket_accept(QIOChannelSocket *ioc, > >> > >> #ifndef WIN32 > >> if (cioc->localAddr.ss_family == AF_UNIX) { > >> - QIO_CHANNEL(cioc)->features |= (1 << > >> QIO_CHANNEL_FEATURE_FD_PASS); > >> + QIO_CHANNEL(cioc)->features |= QIO_CHANNEL_FEATURE_FD_PASS; > >> } > >> #endif /* WIN32 */ > >> > >> @@ -403,7 +403,8 @@ static void qio_channel_socket_finalize(Object *obj) > >> QIOChannelSocket *ioc = QIO_CHANNEL_SOCKET(obj); > >> > >> if (ioc->fd != -1) { > >> - if (QIO_CHANNEL(ioc)->features & QIO_CHANNEL_FEATURE_LISTEN) { > >> + if (qio_channel_has_feature(QIO_CHANNEL(ioc), > >> + QIO_CHANNEL_FEATURE_LISTEN)) { > >> Error *err = NULL; > >> > >> socket_listen_cleanup(ioc->fd, &err); > >> diff --git a/io/channel-tls.c b/io/channel-tls.c > >> index 9a8525c..d22996b 100644 > >> --- a/io/channel-tls.c > >> +++ b/io/channel-tls.c > >> @@ -111,8 +111,8 @@ qio_channel_tls_new_client(QIOChannel *master, > >> ioc = QIO_CHANNEL(tioc); > >> > >> tioc->master = master; > >> - if (master->features & (1 << QIO_CHANNEL_FEATURE_SHUTDOWN)) { > >> - ioc->features |= (1 << QIO_CHANNEL_FEATURE_SHUTDOWN); > >> + if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOWN)) { > >> + ioc->features |= QIO_CHANNEL_FEATURE_SHUTDOWN; > >> } > >> object_ref(OBJECT(master)); > >> > >> diff --git a/io/channel-websock.c b/io/channel-websock.c > >> index 533bd4b..9accd16 100644 > >> --- a/io/channel-websock.c > >> +++ b/io/channel-websock.c > >> @@ -497,8 +497,8 @@ qio_channel_websock_new_server(QIOChannel *master) > >> ioc = QIO_CHANNEL(wioc); > >> > >> wioc->master = master; > >> - if (master->features & (1 << QIO_CHANNEL_FEATURE_SHUTDOWN)) { > >> - ioc->features |= (1 << QIO_CHANNEL_FEATURE_SHUTDOWN); > >> + if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOWN)) { > >> + ioc->features |= QIO_CHANNEL_FEATURE_SHUTDOWN; > >> } > >> object_ref(OBJECT(master)); > >> > >> diff --git a/io/channel.c b/io/channel.c > >> index 923c465..ebe9f86 100644 > >> --- a/io/channel.c > >> +++ b/io/channel.c > >> @@ -23,13 +23,11 @@ > >> #include "qapi/error.h" > >> #include "qemu/coroutine.h" > >> > >> -bool qio_channel_has_feature(QIOChannel *ioc, > >> - QIOChannelFeature feature) > >> +bool qio_channel_has_feature(QIOChannel *ioc, QIOChannelFeature feature) > >> { > >> - return ioc->features & (1 << feature); > >> + return ioc->features & feature; > >> } > >> > >> - > >> ssize_t qio_channel_readv_full(QIOChannel *ioc, > >> const struct iovec *iov, > >> size_t niov, > >> @@ -40,7 +38,7 @@ ssize_t qio_channel_readv_full(QIOChannel *ioc, > >> QIOChannelClass *klass = QIO_CHANNEL_GET_CLASS(ioc); > >> > >> if ((fds || nfds) && > >> - !(ioc->features & (1 << QIO_CHANNEL_FEATURE_FD_PASS))) { > >> + !qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_FD_PASS)) { > >> error_setg_errno(errp, EINVAL, > >> "Channel does not support file descriptor > >> passing"); > >> return -1; > >> @@ -60,7 +58,7 @@ ssize_t qio_channel_writev_full(QIOChannel *ioc, > >> QIOChannelClass *klass = QIO_CHANNEL_GET_CLASS(ioc); > >> > >> if ((fds || nfds) && > >> - !(ioc->features & (1 << QIO_CHANNEL_FEATURE_FD_PASS))) { > >> + !qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_FD_PASS)) { > >> error_setg_errno(errp, EINVAL, > >> "Channel does not support file descriptor > >> passing"); > >> return -1; > >> diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c > >> index 45c13f1..a39abd5 100644 > >> --- a/migration/qemu-file-channel.c > >> +++ b/migration/qemu-file-channel.c > >> @@ -105,8 +105,7 @@ static int channel_shutdown(void *opaque, > >> { > >> QIOChannel *ioc = QIO_CHANNEL(opaque); > >> > >> - if (qio_channel_has_feature(ioc, > >> - QIO_CHANNEL_FEATURE_SHUTDOWN)) { > >> + if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_SHUTDOWN)) { > >> QIOChannelShutdown mode; > >> if (rd && wr) { > >> mode = QIO_CHANNEL_SHUTDOWN_BOTH; > >> diff --git a/qemu-char.c b/qemu-char.c > >> index 8826419..b825331 100644 > >> --- a/qemu-char.c > >> +++ b/qemu-char.c > >> @@ -2787,8 +2787,7 @@ static int tcp_set_msgfds(CharDriverState *chr, int > >> *fds, int num) > >> s->write_msgfds_num = 0; > >> > >> if (!s->connected || > >> - !qio_channel_has_feature(s->ioc, > >> - QIO_CHANNEL_FEATURE_FD_PASS)) { > >> + !qio_channel_has_feature(s->ioc, QIO_CHANNEL_FEATURE_FD_PASS)) { > >> return -1; > >> } > >> > >> -- > >> 1.7.1 > >> > >> > >
On Wed, Sep 28, 2016 at 10:55:33AM +0000, Felipe Franciosi wrote: > Hi Marc, > > > On 27 Sep 2016, at 18:12, Marc-André Lureau <mlureau@redhat.com> wrote: > > > > Hi > > > > ----- Original Message ----- > >> When QIOChannels were introduced in 666a3af9, the feature bits were > >> defined shifted. However, when using them, the code was shifting them > >> again. The incorrect use was consistent until 74b6ce43, where > >> QIO_CHANNEL_FEATURE_LISTEN was defined shifted but tested unshifted. > >> > > > > And by luck, QIO_CHANNEL_FEATURE_LISTEN == (1 << QIO_CHANNEL_FEATURE_LISTEN), so it works :) > > Can you elaborate on that? Maybe I'm missing something obvious, but > I'm confused as to how they are equivalent. They're not equivalent QIO_CHANNEL_FEATURE_LISTEN is (1 << 2) which evaulates to 4 and 4 != (1 << 4). Regards, Daniel
Hi ----- Original Message ----- > Hi > > ----- Original Message ----- > > Hi Marc, > > > > > On 27 Sep 2016, at 18:12, Marc-André Lureau <mlureau@redhat.com> wrote: > > > > > > Hi > > > > > > ----- Original Message ----- > > >> When QIOChannels were introduced in 666a3af9, the feature bits were > > >> defined shifted. However, when using them, the code was shifting them > > >> again. The incorrect use was consistent until 74b6ce43, where > > >> QIO_CHANNEL_FEATURE_LISTEN was defined shifted but tested unshifted. > > >> > > > > > > And by luck, QIO_CHANNEL_FEATURE_LISTEN == (1 << > > > QIO_CHANNEL_FEATURE_LISTEN), so it works :) > > Oops, they are not, QIO_CHANNEL_FEATURE_LISTEN == 4 (I read 2), then I wonder > how could 74b6ce43e work.. > And even 2 wouldn't be true.. (sigh) > > > > Can you elaborate on that? Maybe I'm missing something obvious, but I'm > > confused as to how they are equivalent. > > > > Thanks, > > Felipe > > > > > > > >> This patch fixes all uses of QIOChannel features. They are defined > > >> shifted and therefore set unshifted. It also makes all feature tests to > > >> use the qio_channel_has_feature() function. > > > > > > Looks good, we may want to have it in -stable. > > > > > >> > > >> Signed-off-by: Felipe Franciosi <felipe@nutanix.com> > > > > > > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > > >> --- > > >> io/channel-socket.c | 11 ++++++----- > > >> io/channel-tls.c | 4 ++-- > > >> io/channel-websock.c | 4 ++-- > > >> io/channel.c | 10 ++++------ > > >> migration/qemu-file-channel.c | 3 +-- > > >> qemu-char.c | 3 +-- > > >> 6 files changed, 16 insertions(+), 19 deletions(-) > > >> > > >> diff --git a/io/channel-socket.c b/io/channel-socket.c > > >> index 196a4f1..bb0a0a7 100644 > > >> --- a/io/channel-socket.c > > >> +++ b/io/channel-socket.c > > >> @@ -55,7 +55,7 @@ qio_channel_socket_new(void) > > >> sioc->fd = -1; > > >> > > >> ioc = QIO_CHANNEL(sioc); > > >> - ioc->features |= (1 << QIO_CHANNEL_FEATURE_SHUTDOWN); > > >> + ioc->features |= QIO_CHANNEL_FEATURE_SHUTDOWN; > > >> > > >> #ifdef WIN32 > > >> ioc->event = CreateEvent(NULL, FALSE, FALSE, NULL); > > >> @@ -107,12 +107,12 @@ qio_channel_socket_set_fd(QIOChannelSocket *sioc, > > >> #ifndef WIN32 > > >> if (sioc->localAddr.ss_family == AF_UNIX) { > > >> QIOChannel *ioc = QIO_CHANNEL(sioc); > > >> - ioc->features |= (1 << QIO_CHANNEL_FEATURE_FD_PASS); > > >> + ioc->features |= QIO_CHANNEL_FEATURE_FD_PASS; > > >> } > > >> #endif /* WIN32 */ > > >> if (getsockopt(fd, SOL_SOCKET, SO_ACCEPTCONN, &val, &len) == 0 && > > >> val) > > >> { > > >> QIOChannel *ioc = QIO_CHANNEL(sioc); > > >> - ioc->features |= (1 << QIO_CHANNEL_FEATURE_LISTEN); > > >> + ioc->features |= QIO_CHANNEL_FEATURE_LISTEN; > > >> } > > >> > > >> return 0; > > >> @@ -380,7 +380,7 @@ qio_channel_socket_accept(QIOChannelSocket *ioc, > > >> > > >> #ifndef WIN32 > > >> if (cioc->localAddr.ss_family == AF_UNIX) { > > >> - QIO_CHANNEL(cioc)->features |= (1 << > > >> QIO_CHANNEL_FEATURE_FD_PASS); > > >> + QIO_CHANNEL(cioc)->features |= QIO_CHANNEL_FEATURE_FD_PASS; > > >> } > > >> #endif /* WIN32 */ > > >> > > >> @@ -403,7 +403,8 @@ static void qio_channel_socket_finalize(Object *obj) > > >> QIOChannelSocket *ioc = QIO_CHANNEL_SOCKET(obj); > > >> > > >> if (ioc->fd != -1) { > > >> - if (QIO_CHANNEL(ioc)->features & QIO_CHANNEL_FEATURE_LISTEN) { > > >> + if (qio_channel_has_feature(QIO_CHANNEL(ioc), > > >> + QIO_CHANNEL_FEATURE_LISTEN)) { > > >> Error *err = NULL; > > >> > > >> socket_listen_cleanup(ioc->fd, &err); > > >> diff --git a/io/channel-tls.c b/io/channel-tls.c > > >> index 9a8525c..d22996b 100644 > > >> --- a/io/channel-tls.c > > >> +++ b/io/channel-tls.c > > >> @@ -111,8 +111,8 @@ qio_channel_tls_new_client(QIOChannel *master, > > >> ioc = QIO_CHANNEL(tioc); > > >> > > >> tioc->master = master; > > >> - if (master->features & (1 << QIO_CHANNEL_FEATURE_SHUTDOWN)) { > > >> - ioc->features |= (1 << QIO_CHANNEL_FEATURE_SHUTDOWN); > > >> + if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOWN)) > > >> { > > >> + ioc->features |= QIO_CHANNEL_FEATURE_SHUTDOWN; > > >> } > > >> object_ref(OBJECT(master)); > > >> > > >> diff --git a/io/channel-websock.c b/io/channel-websock.c > > >> index 533bd4b..9accd16 100644 > > >> --- a/io/channel-websock.c > > >> +++ b/io/channel-websock.c > > >> @@ -497,8 +497,8 @@ qio_channel_websock_new_server(QIOChannel *master) > > >> ioc = QIO_CHANNEL(wioc); > > >> > > >> wioc->master = master; > > >> - if (master->features & (1 << QIO_CHANNEL_FEATURE_SHUTDOWN)) { > > >> - ioc->features |= (1 << QIO_CHANNEL_FEATURE_SHUTDOWN); > > >> + if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOWN)) > > >> { > > >> + ioc->features |= QIO_CHANNEL_FEATURE_SHUTDOWN; > > >> } > > >> object_ref(OBJECT(master)); > > >> > > >> diff --git a/io/channel.c b/io/channel.c > > >> index 923c465..ebe9f86 100644 > > >> --- a/io/channel.c > > >> +++ b/io/channel.c > > >> @@ -23,13 +23,11 @@ > > >> #include "qapi/error.h" > > >> #include "qemu/coroutine.h" > > >> > > >> -bool qio_channel_has_feature(QIOChannel *ioc, > > >> - QIOChannelFeature feature) > > >> +bool qio_channel_has_feature(QIOChannel *ioc, QIOChannelFeature > > >> feature) > > >> { > > >> - return ioc->features & (1 << feature); > > >> + return ioc->features & feature; > > >> } > > >> > > >> - > > >> ssize_t qio_channel_readv_full(QIOChannel *ioc, > > >> const struct iovec *iov, > > >> size_t niov, > > >> @@ -40,7 +38,7 @@ ssize_t qio_channel_readv_full(QIOChannel *ioc, > > >> QIOChannelClass *klass = QIO_CHANNEL_GET_CLASS(ioc); > > >> > > >> if ((fds || nfds) && > > >> - !(ioc->features & (1 << QIO_CHANNEL_FEATURE_FD_PASS))) { > > >> + !qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_FD_PASS)) { > > >> error_setg_errno(errp, EINVAL, > > >> "Channel does not support file descriptor > > >> passing"); > > >> return -1; > > >> @@ -60,7 +58,7 @@ ssize_t qio_channel_writev_full(QIOChannel *ioc, > > >> QIOChannelClass *klass = QIO_CHANNEL_GET_CLASS(ioc); > > >> > > >> if ((fds || nfds) && > > >> - !(ioc->features & (1 << QIO_CHANNEL_FEATURE_FD_PASS))) { > > >> + !qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_FD_PASS)) { > > >> error_setg_errno(errp, EINVAL, > > >> "Channel does not support file descriptor > > >> passing"); > > >> return -1; > > >> diff --git a/migration/qemu-file-channel.c > > >> b/migration/qemu-file-channel.c > > >> index 45c13f1..a39abd5 100644 > > >> --- a/migration/qemu-file-channel.c > > >> +++ b/migration/qemu-file-channel.c > > >> @@ -105,8 +105,7 @@ static int channel_shutdown(void *opaque, > > >> { > > >> QIOChannel *ioc = QIO_CHANNEL(opaque); > > >> > > >> - if (qio_channel_has_feature(ioc, > > >> - QIO_CHANNEL_FEATURE_SHUTDOWN)) { > > >> + if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_SHUTDOWN)) { > > >> QIOChannelShutdown mode; > > >> if (rd && wr) { > > >> mode = QIO_CHANNEL_SHUTDOWN_BOTH; > > >> diff --git a/qemu-char.c b/qemu-char.c > > >> index 8826419..b825331 100644 > > >> --- a/qemu-char.c > > >> +++ b/qemu-char.c > > >> @@ -2787,8 +2787,7 @@ static int tcp_set_msgfds(CharDriverState *chr, > > >> int > > >> *fds, int num) > > >> s->write_msgfds_num = 0; > > >> > > >> if (!s->connected || > > >> - !qio_channel_has_feature(s->ioc, > > >> - QIO_CHANNEL_FEATURE_FD_PASS)) { > > >> + !qio_channel_has_feature(s->ioc, QIO_CHANNEL_FEATURE_FD_PASS)) > > >> { > > >> return -1; > > >> } > > >> > > >> -- > > >> 1.7.1 > > >> > > >> > > > > >
diff --git a/io/channel-socket.c b/io/channel-socket.c index 196a4f1..bb0a0a7 100644 --- a/io/channel-socket.c +++ b/io/channel-socket.c @@ -55,7 +55,7 @@ qio_channel_socket_new(void) sioc->fd = -1; ioc = QIO_CHANNEL(sioc); - ioc->features |= (1 << QIO_CHANNEL_FEATURE_SHUTDOWN); + ioc->features |= QIO_CHANNEL_FEATURE_SHUTDOWN; #ifdef WIN32 ioc->event = CreateEvent(NULL, FALSE, FALSE, NULL); @@ -107,12 +107,12 @@ qio_channel_socket_set_fd(QIOChannelSocket *sioc, #ifndef WIN32 if (sioc->localAddr.ss_family == AF_UNIX) { QIOChannel *ioc = QIO_CHANNEL(sioc); - ioc->features |= (1 << QIO_CHANNEL_FEATURE_FD_PASS); + ioc->features |= QIO_CHANNEL_FEATURE_FD_PASS; } #endif /* WIN32 */ if (getsockopt(fd, SOL_SOCKET, SO_ACCEPTCONN, &val, &len) == 0 && val) { QIOChannel *ioc = QIO_CHANNEL(sioc); - ioc->features |= (1 << QIO_CHANNEL_FEATURE_LISTEN); + ioc->features |= QIO_CHANNEL_FEATURE_LISTEN; } return 0; @@ -380,7 +380,7 @@ qio_channel_socket_accept(QIOChannelSocket *ioc, #ifndef WIN32 if (cioc->localAddr.ss_family == AF_UNIX) { - QIO_CHANNEL(cioc)->features |= (1 << QIO_CHANNEL_FEATURE_FD_PASS); + QIO_CHANNEL(cioc)->features |= QIO_CHANNEL_FEATURE_FD_PASS; } #endif /* WIN32 */ @@ -403,7 +403,8 @@ static void qio_channel_socket_finalize(Object *obj) QIOChannelSocket *ioc = QIO_CHANNEL_SOCKET(obj); if (ioc->fd != -1) { - if (QIO_CHANNEL(ioc)->features & QIO_CHANNEL_FEATURE_LISTEN) { + if (qio_channel_has_feature(QIO_CHANNEL(ioc), + QIO_CHANNEL_FEATURE_LISTEN)) { Error *err = NULL; socket_listen_cleanup(ioc->fd, &err); diff --git a/io/channel-tls.c b/io/channel-tls.c index 9a8525c..d22996b 100644 --- a/io/channel-tls.c +++ b/io/channel-tls.c @@ -111,8 +111,8 @@ qio_channel_tls_new_client(QIOChannel *master, ioc = QIO_CHANNEL(tioc); tioc->master = master; - if (master->features & (1 << QIO_CHANNEL_FEATURE_SHUTDOWN)) { - ioc->features |= (1 << QIO_CHANNEL_FEATURE_SHUTDOWN); + if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOWN)) { + ioc->features |= QIO_CHANNEL_FEATURE_SHUTDOWN; } object_ref(OBJECT(master)); diff --git a/io/channel-websock.c b/io/channel-websock.c index 533bd4b..9accd16 100644 --- a/io/channel-websock.c +++ b/io/channel-websock.c @@ -497,8 +497,8 @@ qio_channel_websock_new_server(QIOChannel *master) ioc = QIO_CHANNEL(wioc); wioc->master = master; - if (master->features & (1 << QIO_CHANNEL_FEATURE_SHUTDOWN)) { - ioc->features |= (1 << QIO_CHANNEL_FEATURE_SHUTDOWN); + if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOWN)) { + ioc->features |= QIO_CHANNEL_FEATURE_SHUTDOWN; } object_ref(OBJECT(master)); diff --git a/io/channel.c b/io/channel.c index 923c465..ebe9f86 100644 --- a/io/channel.c +++ b/io/channel.c @@ -23,13 +23,11 @@ #include "qapi/error.h" #include "qemu/coroutine.h" -bool qio_channel_has_feature(QIOChannel *ioc, - QIOChannelFeature feature) +bool qio_channel_has_feature(QIOChannel *ioc, QIOChannelFeature feature) { - return ioc->features & (1 << feature); + return ioc->features & feature; } - ssize_t qio_channel_readv_full(QIOChannel *ioc, const struct iovec *iov, size_t niov, @@ -40,7 +38,7 @@ ssize_t qio_channel_readv_full(QIOChannel *ioc, QIOChannelClass *klass = QIO_CHANNEL_GET_CLASS(ioc); if ((fds || nfds) && - !(ioc->features & (1 << QIO_CHANNEL_FEATURE_FD_PASS))) { + !qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_FD_PASS)) { error_setg_errno(errp, EINVAL, "Channel does not support file descriptor passing"); return -1; @@ -60,7 +58,7 @@ ssize_t qio_channel_writev_full(QIOChannel *ioc, QIOChannelClass *klass = QIO_CHANNEL_GET_CLASS(ioc); if ((fds || nfds) && - !(ioc->features & (1 << QIO_CHANNEL_FEATURE_FD_PASS))) { + !qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_FD_PASS)) { error_setg_errno(errp, EINVAL, "Channel does not support file descriptor passing"); return -1; diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c index 45c13f1..a39abd5 100644 --- a/migration/qemu-file-channel.c +++ b/migration/qemu-file-channel.c @@ -105,8 +105,7 @@ static int channel_shutdown(void *opaque, { QIOChannel *ioc = QIO_CHANNEL(opaque); - if (qio_channel_has_feature(ioc, - QIO_CHANNEL_FEATURE_SHUTDOWN)) { + if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_SHUTDOWN)) { QIOChannelShutdown mode; if (rd && wr) { mode = QIO_CHANNEL_SHUTDOWN_BOTH; diff --git a/qemu-char.c b/qemu-char.c index 8826419..b825331 100644 --- a/qemu-char.c +++ b/qemu-char.c @@ -2787,8 +2787,7 @@ static int tcp_set_msgfds(CharDriverState *chr, int *fds, int num) s->write_msgfds_num = 0; if (!s->connected || - !qio_channel_has_feature(s->ioc, - QIO_CHANNEL_FEATURE_FD_PASS)) { + !qio_channel_has_feature(s->ioc, QIO_CHANNEL_FEATURE_FD_PASS)) { return -1; }
When QIOChannels were introduced in 666a3af9, the feature bits were defined shifted. However, when using them, the code was shifting them again. The incorrect use was consistent until 74b6ce43, where QIO_CHANNEL_FEATURE_LISTEN was defined shifted but tested unshifted. This patch fixes all uses of QIOChannel features. They are defined shifted and therefore set unshifted. It also makes all feature tests to use the qio_channel_has_feature() function. Signed-off-by: Felipe Franciosi <felipe@nutanix.com> --- io/channel-socket.c | 11 ++++++----- io/channel-tls.c | 4 ++-- io/channel-websock.c | 4 ++-- io/channel.c | 10 ++++------ migration/qemu-file-channel.c | 3 +-- qemu-char.c | 3 +-- 6 files changed, 16 insertions(+), 19 deletions(-)