Re: [Qemu-devel] [PATCH] io: Fix double shift usages on QIOChannel features

[Daniel P. Berrange]

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 <address@hidden>
> ---
>  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
-- 
|: 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/ :|