From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50341) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bpCZh-0005Lh-VB for qemu-devel@nongnu.org; Wed, 28 Sep 2016 06:58:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bpCZd-00036S-U5 for qemu-devel@nongnu.org; Wed, 28 Sep 2016 06:58:53 -0400 Received: from mx4-phx2.redhat.com ([209.132.183.25]:37132) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bpCZd-00035z-Iz for qemu-devel@nongnu.org; Wed, 28 Sep 2016 06:58:49 -0400 Date: Wed, 28 Sep 2016 06:58:48 -0400 (EDT) From: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau Message-ID: <921443124.124128.1475060328575.JavaMail.zimbra@redhat.com> In-Reply-To: References: <1474994958-6007-1-git-send-email-felipe@nutanix.com> <1484304137.546185.1474996345417.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 Marc, >=20 > > On 27 Sep 2016, at 18:12, Marc-Andr=C3=A9 Lureau w= rote: > >=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 them > >> 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 :) Oops, they are not, QIO_CHANNEL_FEATURE_LISTEN =3D=3D 4 (I read 2), then I = wonder how could 74b6ce43e work.. >=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 t= o > >> 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 *sioc= , > >> #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_PASS; > >> } > >> #endif /* WIN32 */ > >>=20 > >> @@ -403,7 +403,8 @@ static void qio_channel_socket_finalize(Object *ob= j) > >> 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_SHUTDOWN)= ) { > >> + 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 *master) > >> 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_SHUTDOWN)= ) { > >> + 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 featu= re) > >> { > >> - 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-chann= el.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_PASS)= ) { > >> return -1; > >> } > >>=20 > >> -- > >> 1.7.1 > >>=20 > >>=20 >=20 >=20