All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v2 0/3] io: Various fixes around QIOChannel Features
@ 2016-09-28 12:36 Felipe Franciosi
  2016-09-28 12:36 ` [Qemu-devel] [PATCH v2 1/3] io: Fix double shift usages on QIOChannel features Felipe Franciosi
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Felipe Franciosi @ 2016-09-28 12:36 UTC (permalink / raw)
  To: berrange, marcandre.lureau; +Cc: pbonzini, qemu-devel, Felipe Franciosi

This series include three patches around the utilisation of QIOChannel
features. The first patch actually fixes a bug, while the other two
makes the test/set of features consistent by using helper functions.

Changes from v1:
 - Fix two pointer declarations (were missing a star)
 - Fix a call to _set_features() which should be _set_feature()

Felipe Franciosi (3):
  io: Fix double shift usages on QIOChannel features
  io: Use qio_channel_has_feature() where applicable
  io: Introduce a qio_channel_set_feature() helper

 include/io/channel.h |   16 +++++++++++++---
 io/channel-socket.c  |   12 +++++++-----
 io/channel-tls.c     |    4 ++--
 io/channel-websock.c |    4 ++--
 io/channel.c         |   11 +++++++++--
 5 files changed, 33 insertions(+), 14 deletions(-)

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Qemu-devel] [PATCH v2 1/3] io: Fix double shift usages on QIOChannel features
  2016-09-28 12:36 [Qemu-devel] [PATCH v2 0/3] io: Various fixes around QIOChannel Features Felipe Franciosi
@ 2016-09-28 12:36 ` Felipe Franciosi
  2016-09-28 12:44   ` Daniel P. Berrange
  2016-09-28 12:36 ` [Qemu-devel] [PATCH v2 2/3] io: Use qio_channel_has_feature() where applicable Felipe Franciosi
  2016-09-28 12:36 ` [Qemu-devel] [PATCH v2 3/3] io: Introduce a qio_channel_set_feature() helper Felipe Franciosi
  2 siblings, 1 reply; 7+ messages in thread
From: Felipe Franciosi @ 2016-09-28 12:36 UTC (permalink / raw)
  To: berrange, marcandre.lureau; +Cc: pbonzini, qemu-devel, Felipe Franciosi

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);
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [Qemu-devel] [PATCH v2 2/3] io: Use qio_channel_has_feature() where applicable
  2016-09-28 12:36 [Qemu-devel] [PATCH v2 0/3] io: Various fixes around QIOChannel Features Felipe Franciosi
  2016-09-28 12:36 ` [Qemu-devel] [PATCH v2 1/3] io: Fix double shift usages on QIOChannel features Felipe Franciosi
@ 2016-09-28 12:36 ` Felipe Franciosi
  2016-09-28 12:36 ` [Qemu-devel] [PATCH v2 3/3] io: Introduce a qio_channel_set_feature() helper Felipe Franciosi
  2 siblings, 0 replies; 7+ messages in thread
From: Felipe Franciosi @ 2016-09-28 12:36 UTC (permalink / raw)
  To: berrange, marcandre.lureau; +Cc: pbonzini, qemu-devel, Felipe Franciosi

Parts of the code have been testing QIOChannel features directly with a
logical AND. This patch makes it all consistent by using the
qio_channel_has_feature() function to test if a feature is present.

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

diff --git a/io/channel-socket.c b/io/channel-socket.c
index 6710b2e..8fc6e5a 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -403,7 +403,8 @@ static void qio_channel_socket_finalize(Object *obj)
     QIOChannelSocket *ioc = QIO_CHANNEL_SOCKET(obj);
 
     if (ioc->fd != -1) {
-        if (QIO_CHANNEL(ioc)->features & (1 << QIO_CHANNEL_FEATURE_LISTEN)) {
+        QIOChannel *ioc_local = QIO_CHANNEL(ioc);
+        if (qio_channel_has_feature(ioc_local, QIO_CHANNEL_FEATURE_LISTEN)) {
             Error *err = NULL;
 
             socket_listen_cleanup(ioc->fd, &err);
diff --git a/io/channel-tls.c b/io/channel-tls.c
index 9a8525c..f7bb0e3 100644
--- a/io/channel-tls.c
+++ b/io/channel-tls.c
@@ -111,7 +111,7 @@ qio_channel_tls_new_client(QIOChannel *master,
     ioc = QIO_CHANNEL(tioc);
 
     tioc->master = master;
-    if (master->features & (1 << QIO_CHANNEL_FEATURE_SHUTDOWN)) {
+    if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOWN)) {
         ioc->features |= (1 << QIO_CHANNEL_FEATURE_SHUTDOWN);
     }
     object_ref(OBJECT(master));
diff --git a/io/channel-websock.c b/io/channel-websock.c
index 533bd4b..75df03e 100644
--- a/io/channel-websock.c
+++ b/io/channel-websock.c
@@ -497,7 +497,7 @@ qio_channel_websock_new_server(QIOChannel *master)
     ioc = QIO_CHANNEL(wioc);
 
     wioc->master = master;
-    if (master->features & (1 << QIO_CHANNEL_FEATURE_SHUTDOWN)) {
+    if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOWN)) {
         ioc->features |= (1 << QIO_CHANNEL_FEATURE_SHUTDOWN);
     }
     object_ref(OBJECT(master));
diff --git a/io/channel.c b/io/channel.c
index 923c465..e50325c 100644
--- a/io/channel.c
+++ b/io/channel.c
@@ -40,7 +40,7 @@ ssize_t qio_channel_readv_full(QIOChannel *ioc,
     QIOChannelClass *klass = QIO_CHANNEL_GET_CLASS(ioc);
 
     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 +60,7 @@ ssize_t qio_channel_writev_full(QIOChannel *ioc,
     QIOChannelClass *klass = QIO_CHANNEL_GET_CLASS(ioc);
 
     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;
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [Qemu-devel] [PATCH v2 3/3] io: Introduce a qio_channel_set_feature() helper
  2016-09-28 12:36 [Qemu-devel] [PATCH v2 0/3] io: Various fixes around QIOChannel Features Felipe Franciosi
  2016-09-28 12:36 ` [Qemu-devel] [PATCH v2 1/3] io: Fix double shift usages on QIOChannel features Felipe Franciosi
  2016-09-28 12:36 ` [Qemu-devel] [PATCH v2 2/3] io: Use qio_channel_has_feature() where applicable Felipe Franciosi
@ 2016-09-28 12:36 ` Felipe Franciosi
  2 siblings, 0 replies; 7+ messages in thread
From: Felipe Franciosi @ 2016-09-28 12:36 UTC (permalink / raw)
  To: berrange, marcandre.lureau; +Cc: pbonzini, qemu-devel, Felipe Franciosi

Testing QIOChannel feature support can be done with a helper called
qio_channel_has_feature(). Setting feature support, however, was
done manually with a logical OR. This patch introduces a new helper
called qio_channel_set_feature() and makes use of it where applicable.

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

diff --git a/include/io/channel.h b/include/io/channel.h
index 5368604..cf1c622 100644
--- a/include/io/channel.h
+++ b/include/io/channel.h
@@ -149,6 +149,16 @@ bool qio_channel_has_feature(QIOChannel *ioc,
                              QIOChannelFeature feature);
 
 /**
+ * qio_channel_set_feature:
+ * @ioc: the channel object
+ * @feature: the feature to set support for
+ *
+ * Add channel support for the feature named in @feature.
+ */
+void qio_channel_set_feature(QIOChannel *ioc,
+                             QIOChannelFeature feature);
+
+/**
  * qio_channel_readv_full:
  * @ioc: the channel object
  * @iov: the array of memory regions to read data into
diff --git a/io/channel-socket.c b/io/channel-socket.c
index 8fc6e5a..75cbca3 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -55,7 +55,7 @@ qio_channel_socket_new(void)
     sioc->fd = -1;
 
     ioc = QIO_CHANNEL(sioc);
-    ioc->features |= (1 << QIO_CHANNEL_FEATURE_SHUTDOWN);
+    qio_channel_set_feature(ioc, QIO_CHANNEL_FEATURE_SHUTDOWN);
 
 #ifdef WIN32
     ioc->event = CreateEvent(NULL, FALSE, FALSE, NULL);
@@ -107,12 +107,12 @@ qio_channel_socket_set_fd(QIOChannelSocket *sioc,
 #ifndef WIN32
     if (sioc->localAddr.ss_family == AF_UNIX) {
         QIOChannel *ioc = QIO_CHANNEL(sioc);
-        ioc->features |= (1 << QIO_CHANNEL_FEATURE_FD_PASS);
+        qio_channel_set_feature(ioc, QIO_CHANNEL_FEATURE_FD_PASS);
     }
 #endif /* WIN32 */
     if (getsockopt(fd, SOL_SOCKET, SO_ACCEPTCONN, &val, &len) == 0 && val) {
         QIOChannel *ioc = QIO_CHANNEL(sioc);
-        ioc->features |= (1 << QIO_CHANNEL_FEATURE_LISTEN);
+        qio_channel_set_feature(ioc, QIO_CHANNEL_FEATURE_LISTEN);
     }
 
     return 0;
@@ -380,7 +380,8 @@ qio_channel_socket_accept(QIOChannelSocket *ioc,
 
 #ifndef WIN32
     if (cioc->localAddr.ss_family == AF_UNIX) {
-        QIO_CHANNEL(cioc)->features |= (1 << QIO_CHANNEL_FEATURE_FD_PASS);
+        QIOChannel *ioc_local = QIO_CHANNEL(cioc);
+        qio_channel_set_feature(ioc_local, QIO_CHANNEL_FEATURE_FD_PASS);
     }
 #endif /* WIN32 */
 
diff --git a/io/channel-tls.c b/io/channel-tls.c
index f7bb0e3..d24dc8c 100644
--- a/io/channel-tls.c
+++ b/io/channel-tls.c
@@ -112,7 +112,7 @@ qio_channel_tls_new_client(QIOChannel *master,
 
     tioc->master = master;
     if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOWN)) {
-        ioc->features |= (1 << QIO_CHANNEL_FEATURE_SHUTDOWN);
+        qio_channel_set_feature(ioc, QIO_CHANNEL_FEATURE_SHUTDOWN);
     }
     object_ref(OBJECT(master));
 
diff --git a/io/channel-websock.c b/io/channel-websock.c
index 75df03e..f45bced 100644
--- a/io/channel-websock.c
+++ b/io/channel-websock.c
@@ -498,7 +498,7 @@ qio_channel_websock_new_server(QIOChannel *master)
 
     wioc->master = master;
     if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOWN)) {
-        ioc->features |= (1 << QIO_CHANNEL_FEATURE_SHUTDOWN);
+        qio_channel_set_feature(ioc, QIO_CHANNEL_FEATURE_SHUTDOWN);
     }
     object_ref(OBJECT(master));
 
diff --git a/io/channel.c b/io/channel.c
index e50325c..d1f1ae5 100644
--- a/io/channel.c
+++ b/io/channel.c
@@ -30,6 +30,13 @@ bool qio_channel_has_feature(QIOChannel *ioc,
 }
 
 
+void qio_channel_set_feature(QIOChannel *ioc,
+                             QIOChannelFeature feature)
+{
+    ioc->features |= (1 << feature);
+}
+
+
 ssize_t qio_channel_readv_full(QIOChannel *ioc,
                                const struct iovec *iov,
                                size_t niov,
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/3] io: Fix double shift usages on QIOChannel features
  2016-09-28 12:36 ` [Qemu-devel] [PATCH v2 1/3] io: Fix double shift usages on QIOChannel features Felipe Franciosi
@ 2016-09-28 12:44   ` Daniel P. Berrange
  2016-09-28 14:49     ` Felipe Franciosi
  0 siblings, 1 reply; 7+ messages in thread
From: Daniel P. Berrange @ 2016-09-28 12:44 UTC (permalink / raw)
  To: Felipe Franciosi; +Cc: marcandre.lureau, pbonzini, qemu-devel

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
-- 
|: 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/ :|

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/3] io: Fix double shift usages on QIOChannel features
  2016-09-28 12:44   ` Daniel P. Berrange
@ 2016-09-28 14:49     ` Felipe Franciosi
  2016-09-28 15:00       ` Daniel P. Berrange
  0 siblings, 1 reply; 7+ messages in thread
From: Felipe Franciosi @ 2016-09-28 14:49 UTC (permalink / raw)
  To: Daniel P. Berrange; +Cc: Marc-Andre Lureau, Paolo Bonzini, qemu-devel

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/ :|

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/3] io: Fix double shift usages on QIOChannel features
  2016-09-28 14:49     ` Felipe Franciosi
@ 2016-09-28 15:00       ` Daniel P. Berrange
  0 siblings, 0 replies; 7+ messages in thread
From: Daniel P. Berrange @ 2016-09-28 15:00 UTC (permalink / raw)
  To: Felipe Franciosi; +Cc: Marc-Andre Lureau, Paolo Bonzini, qemu-devel

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
-- 
|: 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/ :|

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2016-09-28 15:00 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-28 12:36 [Qemu-devel] [PATCH v2 0/3] io: Various fixes around QIOChannel Features Felipe Franciosi
2016-09-28 12:36 ` [Qemu-devel] [PATCH v2 1/3] io: Fix double shift usages on QIOChannel features Felipe Franciosi
2016-09-28 12:44   ` Daniel P. Berrange
2016-09-28 14:49     ` Felipe Franciosi
2016-09-28 15:00       ` Daniel P. Berrange
2016-09-28 12:36 ` [Qemu-devel] [PATCH v2 2/3] io: Use qio_channel_has_feature() where applicable Felipe Franciosi
2016-09-28 12:36 ` [Qemu-devel] [PATCH v2 3/3] io: Introduce a qio_channel_set_feature() helper Felipe Franciosi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.