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 <[email protected]> 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 <[email protected]> > >> --- > >> 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/ :|
