From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52928) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bpChA-0002Gh-W6 for qemu-devel@nongnu.org; Wed, 28 Sep 2016 07:06:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bpCh6-0006GL-TZ for qemu-devel@nongnu.org; Wed, 28 Sep 2016 07:06:36 -0400 Received: from mx6-phx2.redhat.com ([209.132.183.39]:55952) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bpCh6-0006G5-K0 for qemu-devel@nongnu.org; Wed, 28 Sep 2016 07:06:32 -0400 Date: Wed, 28 Sep 2016 07:06:31 -0400 (EDT) From: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau Message-ID: <775506363.124552.1475060791167.JavaMail.zimbra@redhat.com> In-Reply-To: <921443124.124128.1475060328575.JavaMail.zimbra@redhat.com> References: <1474994958-6007-1-git-send-email-felipe@nutanix.com> <1484304137.546185.1474996345417.JavaMail.zimbra@redhat.com> <921443124.124128.1475060328575.JavaMail.zimbra@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] io: Fix double shift usages on QIOChannel features List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Felipe Franciosi Cc: "Daniel P. Berrange" , Marc-Andre Lureau , Paolo Bonzini , qemu-devel@nongnu.org Hi ----- Original Message ----- > Hi >=20 > ----- Original Message ----- > > Hi Marc, > >=20 > > > On 27 Sep 2016, at 18:12, Marc-Andr=C3=A9 Lureau = wrote: > > >=20 > > > Hi > > >=20 > > > ----- Original Message ----- > > >> When QIOChannels were introduced in 666a3af9, the feature bits were > > >> defined shifted. However, when using them, the code was shifting the= m > > >> again. The incorrect use was consistent until 74b6ce43, where > > >> QIO_CHANNEL_FEATURE_LISTEN was defined shifted but tested unshifted. > > >>=20 > > >=20 > > > And by luck, QIO_CHANNEL_FEATURE_LISTEN =3D=3D (1 << > > > QIO_CHANNEL_FEATURE_LISTEN), so it works :) >=20 > Oops, they are not, QIO_CHANNEL_FEATURE_LISTEN =3D=3D 4 (I read 2), then = I wonder > how could 74b6ce43e work.. >=20 And even 2 wouldn't be true.. (sigh) > >=20 > > Can you elaborate on that? Maybe I'm missing something obvious, but I'm > > confused as to how they are equivalent. > >=20 > > Thanks, > > Felipe > >=20 > > >=20 > > >> 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. > > >=20 > > > Looks good, we may want to have it in -stable. > > >=20 > > >>=20 > > >> Signed-off-by: Felipe Franciosi > > >=20 > > > Reviewed-by: Marc-Andr=C3=A9 Lureau > > >=20 > > >> --- > > >> 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(-) > > >>=20 > > >> 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 =3D -1; > > >>=20 > > >> ioc =3D QIO_CHANNEL(sioc); > > >> - ioc->features |=3D (1 << QIO_CHANNEL_FEATURE_SHUTDOWN); > > >> + ioc->features |=3D QIO_CHANNEL_FEATURE_SHUTDOWN; > > >>=20 > > >> #ifdef WIN32 > > >> ioc->event =3D CreateEvent(NULL, FALSE, FALSE, NULL); > > >> @@ -107,12 +107,12 @@ qio_channel_socket_set_fd(QIOChannelSocket *si= oc, > > >> #ifndef WIN32 > > >> if (sioc->localAddr.ss_family =3D=3D AF_UNIX) { > > >> QIOChannel *ioc =3D QIO_CHANNEL(sioc); > > >> - ioc->features |=3D (1 << QIO_CHANNEL_FEATURE_FD_PASS); > > >> + ioc->features |=3D QIO_CHANNEL_FEATURE_FD_PASS; > > >> } > > >> #endif /* WIN32 */ > > >> if (getsockopt(fd, SOL_SOCKET, SO_ACCEPTCONN, &val, &len) =3D=3D= 0 && > > >> val) > > >> { > > >> QIOChannel *ioc =3D QIO_CHANNEL(sioc); > > >> - ioc->features |=3D (1 << QIO_CHANNEL_FEATURE_LISTEN); > > >> + ioc->features |=3D QIO_CHANNEL_FEATURE_LISTEN; > > >> } > > >>=20 > > >> return 0; > > >> @@ -380,7 +380,7 @@ qio_channel_socket_accept(QIOChannelSocket *ioc, > > >>=20 > > >> #ifndef WIN32 > > >> if (cioc->localAddr.ss_family =3D=3D AF_UNIX) { > > >> - QIO_CHANNEL(cioc)->features |=3D (1 << > > >> QIO_CHANNEL_FEATURE_FD_PASS); > > >> + QIO_CHANNEL(cioc)->features |=3D QIO_CHANNEL_FEATURE_FD_PAS= S; > > >> } > > >> #endif /* WIN32 */ > > >>=20 > > >> @@ -403,7 +403,8 @@ static void qio_channel_socket_finalize(Object *= obj) > > >> QIOChannelSocket *ioc =3D QIO_CHANNEL_SOCKET(obj); > > >>=20 > > >> if (ioc->fd !=3D -1) { > > >> - if (QIO_CHANNEL(ioc)->features & QIO_CHANNEL_FEATURE_LISTEN= ) { > > >> + if (qio_channel_has_feature(QIO_CHANNEL(ioc), > > >> + QIO_CHANNEL_FEATURE_LISTEN)) { > > >> Error *err =3D NULL; > > >>=20 > > >> 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 =3D QIO_CHANNEL(tioc); > > >>=20 > > >> tioc->master =3D master; > > >> - if (master->features & (1 << QIO_CHANNEL_FEATURE_SHUTDOWN)) { > > >> - ioc->features |=3D (1 << QIO_CHANNEL_FEATURE_SHUTDOWN); > > >> + if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOW= N)) > > >> { > > >> + ioc->features |=3D QIO_CHANNEL_FEATURE_SHUTDOWN; > > >> } > > >> object_ref(OBJECT(master)); > > >>=20 > > >> 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 *maste= r) > > >> ioc =3D QIO_CHANNEL(wioc); > > >>=20 > > >> wioc->master =3D master; > > >> - if (master->features & (1 << QIO_CHANNEL_FEATURE_SHUTDOWN)) { > > >> - ioc->features |=3D (1 << QIO_CHANNEL_FEATURE_SHUTDOWN); > > >> + if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOW= N)) > > >> { > > >> + ioc->features |=3D QIO_CHANNEL_FEATURE_SHUTDOWN; > > >> } > > >> object_ref(OBJECT(master)); > > >>=20 > > >> 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" > > >>=20 > > >> -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; > > >> } > > >>=20 > > >> - > > >> 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 =3D QIO_CHANNEL_GET_CLASS(ioc); > > >>=20 > > >> 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 =3D QIO_CHANNEL_GET_CLASS(ioc); > > >>=20 > > >> 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 =3D QIO_CHANNEL(opaque); > > >>=20 > > >> - 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 =3D 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 =3D 0; > > >>=20 > > >> if (!s->connected || > > >> - !qio_channel_has_feature(s->ioc, > > >> - QIO_CHANNEL_FEATURE_FD_PASS)) { > > >> + !qio_channel_has_feature(s->ioc, QIO_CHANNEL_FEATURE_FD_PAS= S)) > > >> { > > >> return -1; > > >> } > > >>=20 > > >> -- > > >> 1.7.1 > > >>=20 > > >>=20 > >=20 > >=20 >=20