diff mbox

[v2,1/3] io: Fix double shift usages on QIOChannel features

Message ID 1475066167-17635-2-git-send-email-felipe@nutanix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Felipe Franciosi Sept. 28, 2016, 12:36 p.m. UTC 
When QIOChannels were introduced in 666a3af9, the feature bits were
already 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 changes the definition to be unshifted and fixes the
incorrect usage introduced on 74b6ce43.

Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
---
 include/io/channel.h |    6 +++---
 io/channel-socket.c  |    2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

Comments

Daniel P. Berrangé Sept. 28, 2016, 12:44 p.m. UTC | #1 
On Wed, Sep 28, 2016 at 05:36:05AM -0700, Felipe Franciosi wrote:
> When QIOChannels were introduced in 666a3af9, the feature bits were
> already 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 changes the definition to be unshifted and fixes the
> incorrect usage introduced on 74b6ce43.
> 
> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
> ---
>  include/io/channel.h |    6 +++---
>  io/channel-socket.c  |    2 +-
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/include/io/channel.h b/include/io/channel.h
> index 752e89f..5368604 100644
> --- a/include/io/channel.h
> +++ b/include/io/channel.h
> @@ -40,9 +40,9 @@ typedef struct QIOChannelClass QIOChannelClass;
>  typedef enum QIOChannelFeature QIOChannelFeature;
>  
>  enum QIOChannelFeature {
> -    QIO_CHANNEL_FEATURE_FD_PASS  = (1 << 0),
> -    QIO_CHANNEL_FEATURE_SHUTDOWN = (1 << 1),
> -    QIO_CHANNEL_FEATURE_LISTEN   = (1 << 2),
> +    QIO_CHANNEL_FEATURE_FD_PASS,
> +    QIO_CHANNEL_FEATURE_SHUTDOWN,
> +    QIO_CHANNEL_FEATURE_LISTEN,
>  };
>  
>  
> diff --git a/io/channel-socket.c b/io/channel-socket.c
> index 196a4f1..6710b2e 100644
> --- a/io/channel-socket.c
> +++ b/io/channel-socket.c
> @@ -403,7 +403,7 @@ 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(ioc)->features & (1 << QIO_CHANNEL_FEATURE_LISTEN)) {
>              Error *err = NULL;
>  
>              socket_listen_cleanup(ioc->fd, &err);

The change looks good, but this patch is missing additions to the
various tests/test-io-channel-*.c files, to validate that we correctly
report the SHUTDOWN/LISTEN features being set.

Any test addition should fail on current git master, and succeed with
this fix applied.

Regards,
Daniel
Felipe Franciosi Sept. 28, 2016, 2:49 p.m. UTC | #2 
Hi Daniel,

> On 28 Sep 2016, at 13:44, Daniel P. Berrange <berrange@redhat.com> wrote:
> 
> On Wed, Sep 28, 2016 at 05:36:05AM -0700, Felipe Franciosi wrote:
>> When QIOChannels were introduced in 666a3af9, the feature bits were
>> already 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 changes the definition to be unshifted and fixes the
>> incorrect usage introduced on 74b6ce43.
>> 
>> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
>> ---
>> include/io/channel.h |    6 +++---
>> io/channel-socket.c  |    2 +-
>> 2 files changed, 4 insertions(+), 4 deletions(-)
>> 
>> diff --git a/include/io/channel.h b/include/io/channel.h
>> index 752e89f..5368604 100644
>> --- a/include/io/channel.h
>> +++ b/include/io/channel.h
>> @@ -40,9 +40,9 @@ typedef struct QIOChannelClass QIOChannelClass;
>> typedef enum QIOChannelFeature QIOChannelFeature;
>> 
>> enum QIOChannelFeature {
>> -    QIO_CHANNEL_FEATURE_FD_PASS  = (1 << 0),
>> -    QIO_CHANNEL_FEATURE_SHUTDOWN = (1 << 1),
>> -    QIO_CHANNEL_FEATURE_LISTEN   = (1 << 2),
>> +    QIO_CHANNEL_FEATURE_FD_PASS,
>> +    QIO_CHANNEL_FEATURE_SHUTDOWN,
>> +    QIO_CHANNEL_FEATURE_LISTEN,
>> };
>> 
>> 
>> diff --git a/io/channel-socket.c b/io/channel-socket.c
>> index 196a4f1..6710b2e 100644
>> --- a/io/channel-socket.c
>> +++ b/io/channel-socket.c
>> @@ -403,7 +403,7 @@ 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(ioc)->features & (1 << QIO_CHANNEL_FEATURE_LISTEN)) {
>>             Error *err = NULL;
>> 
>>             socket_listen_cleanup(ioc->fd, &err);
> 
> The change looks good, but this patch is missing additions to the
> various tests/test-io-channel-*.c files, to validate that we correctly
> report the SHUTDOWN/LISTEN features being set.
> 
> Any test addition should fail on current git master, and succeed with
> this fix applied.

From master, if I run "make check", I'm already bumping into errors:

----------------8<----------------
  CC    tests/io-channel-helpers.o
  LINK  tests/test-io-channel-socket
GTESTER tests/test-io-channel-socket
  CC    tests/test-io-channel-file.o
  LINK  tests/test-io-channel-file
GTESTER tests/test-io-channel-file
  CC    tests/test-io-channel-tls.o
  LINK  tests/test-io-channel-tls
GTESTER tests/test-io-channel-tls
test-io-channel-tls: ath.c:193: _gcry_ath_mutex_lock: Assertion `*lock == ((ath_mutex_t) 0)' failed.
GTester: last random seed: R02S9b33b323b6d5e03d003037e20885bc0a
make: *** [check-tests/test-io-channel-tls] Error 1
----------------8<---------------- 

I'm very unfamiliar with qemu's test framework. Are you able to look into this? I think you know more about the tls io channel work than me.

Do you need the tests amended to take this series or can we do that separately? Given that this is currently (quite) broken, I'd prioritise taking the fix and working on the unit tests afterwards.

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/ :|
Daniel P. Berrangé Sept. 28, 2016, 3 p.m. UTC | #3 
On Wed, Sep 28, 2016 at 02:49:45PM +0000, Felipe Franciosi wrote:
> Hi Daniel,
> 
> > On 28 Sep 2016, at 13:44, Daniel P. Berrange <berrange@redhat.com> wrote:
> > 
> > On Wed, Sep 28, 2016 at 05:36:05AM -0700, Felipe Franciosi wrote:
> >> When QIOChannels were introduced in 666a3af9, the feature bits were
> >> already 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 changes the definition to be unshifted and fixes the
> >> incorrect usage introduced on 74b6ce43.
> >> 
> >> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
> >> ---
> >> include/io/channel.h |    6 +++---
> >> io/channel-socket.c  |    2 +-
> >> 2 files changed, 4 insertions(+), 4 deletions(-)
> >> 
> >> diff --git a/include/io/channel.h b/include/io/channel.h
> >> index 752e89f..5368604 100644
> >> --- a/include/io/channel.h
> >> +++ b/include/io/channel.h
> >> @@ -40,9 +40,9 @@ typedef struct QIOChannelClass QIOChannelClass;
> >> typedef enum QIOChannelFeature QIOChannelFeature;
> >> 
> >> enum QIOChannelFeature {
> >> -    QIO_CHANNEL_FEATURE_FD_PASS  = (1 << 0),
> >> -    QIO_CHANNEL_FEATURE_SHUTDOWN = (1 << 1),
> >> -    QIO_CHANNEL_FEATURE_LISTEN   = (1 << 2),
> >> +    QIO_CHANNEL_FEATURE_FD_PASS,
> >> +    QIO_CHANNEL_FEATURE_SHUTDOWN,
> >> +    QIO_CHANNEL_FEATURE_LISTEN,
> >> };
> >> 
> >> 
> >> diff --git a/io/channel-socket.c b/io/channel-socket.c
> >> index 196a4f1..6710b2e 100644
> >> --- a/io/channel-socket.c
> >> +++ b/io/channel-socket.c
> >> @@ -403,7 +403,7 @@ 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(ioc)->features & (1 << QIO_CHANNEL_FEATURE_LISTEN)) {
> >>             Error *err = NULL;
> >> 
> >>             socket_listen_cleanup(ioc->fd, &err);
> > 
> > The change looks good, but this patch is missing additions to the
> > various tests/test-io-channel-*.c files, to validate that we correctly
> > report the SHUTDOWN/LISTEN features being set.
> > 
> > Any test addition should fail on current git master, and succeed with
> > this fix applied.
> 
> From master, if I run "make check", I'm already bumping into errors:
> 
> ----------------8<----------------
>   CC    tests/io-channel-helpers.o
>   LINK  tests/test-io-channel-socket
> GTESTER tests/test-io-channel-socket
>   CC    tests/test-io-channel-file.o
>   LINK  tests/test-io-channel-file
> GTESTER tests/test-io-channel-file
>   CC    tests/test-io-channel-tls.o
>   LINK  tests/test-io-channel-tls
> GTESTER tests/test-io-channel-tls
> test-io-channel-tls: ath.c:193: _gcry_ath_mutex_lock: Assertion `*lock == ((ath_mutex_t) 0)' failed.
> GTester: last random seed: R02S9b33b323b6d5e03d003037e20885bc0a
> make: *** [check-tests/test-io-channel-tls] Error 1
> ----------------8<----------------
> I'm very unfamiliar with qemu's test framework. Are you able to look into this? I think you know more about the tls io channel work than me.

Oh that failure will be a missing call to g_assert(qcrypto_init(NULL) == 0);
in that test case. I'll take care of fixing this test case.

> Do you need the tests amended to take this series or can we do that
> separately? Given that this is currently (quite) broken, I'd prioritise
> taking the fix and working on the unit tests afterwards.

Just ignore the test-io-channel-tls case.

Adding to test-io-channel-socket.c is the most important one. You
can build & run that individual test with

  $ make tests/test-io-channel-socket && tests/test-io-channel-socket

Regards,
Daniel
diff mbox

Patch

diff --git a/include/io/channel.h b/include/io/channel.h
index 752e89f..5368604 100644
--- a/include/io/channel.h
+++ b/include/io/channel.h
@@ -40,9 +40,9 @@  typedef struct QIOChannelClass QIOChannelClass;
 typedef enum QIOChannelFeature QIOChannelFeature;
 
 enum QIOChannelFeature {
-    QIO_CHANNEL_FEATURE_FD_PASS  = (1 << 0),
-    QIO_CHANNEL_FEATURE_SHUTDOWN = (1 << 1),
-    QIO_CHANNEL_FEATURE_LISTEN   = (1 << 2),
+    QIO_CHANNEL_FEATURE_FD_PASS,
+    QIO_CHANNEL_FEATURE_SHUTDOWN,
+    QIO_CHANNEL_FEATURE_LISTEN,
 };
 
 
diff --git a/io/channel-socket.c b/io/channel-socket.c
index 196a4f1..6710b2e 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -403,7 +403,7 @@  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(ioc)->features & (1 << QIO_CHANNEL_FEATURE_LISTEN)) {
             Error *err = NULL;
 
             socket_listen_cleanup(ioc->fd, &err);