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

[Daniel P. Berrange]

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