[Qemu-devel] [PATCH] ccid-card-emulated: use EventNotifier

[Qemu-devel] [PATCH] ccid-card-emulated: use EventNotifier

[Paolo Bonzini]

Shut up Coverity's complaint about unchecked fcntl return values,
and especially make the code simpler and more efficient.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/usb/ccid-card-emulated.c | 29 ++++++++++-------------------
 1 file changed, 10 insertions(+), 19 deletions(-)

diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c
index 7213c89..aa1c37a 100644
--- a/hw/usb/ccid-card-emulated.c
+++ b/hw/usb/ccid-card-emulated.c
@@ -126,7 +126,7 @@ struct EmulatedState {
     QemuMutex vreader_mutex; /* and guest_apdu_list mutex */
     QemuMutex handle_apdu_mutex;
     QemuCond handle_apdu_cond;
-    int      pipe[2];
+    EventNotifier notifier;
     int      quit_apdu_thread;
     QemuThread apdu_thread_id;
 };
@@ -162,9 +162,7 @@ static void emulated_push_event(EmulatedState *card, EmulEvent *event)
     qemu_mutex_lock(&card->event_list_mutex);
     QSIMPLEQ_INSERT_TAIL(&(card->event_list), event, entry);
     qemu_mutex_unlock(&card->event_list_mutex);
-    if (write(card->pipe[1], card, 1) != 1) {
-        DPRINTF(card, 1, "write to pipe failed\n");
-    }
+    event_notifier_set(&card->notifier);
 }
 
 static void emulated_push_type(EmulatedState *card, uint32_t type)
@@ -358,16 +356,12 @@ static void *event_thread(void *arg)
     return NULL;
 }
 
-static void pipe_read(void *opaque)
+static void card_event_handler(EventNotifier *notifier)
 {
-    EmulatedState *card = opaque;
+    EmulatedState *card = container_of(notifier, EmulatedState, notifier);
     EmulEvent *event, *next;
-    char dummy;
-    int len;
 
-    do {
-        len = read(card->pipe[0], &dummy, sizeof(dummy));
-    } while (len == sizeof(dummy));
+    event_notifier_test_and_clear(&card->notifier);
     qemu_mutex_lock(&card->event_list_mutex);
     QSIMPLEQ_FOREACH_SAFE(event, &card->event_list, entry, next) {
         DPRINTF(card, 2, "event %s\n", emul_event_to_string(event->p.gen.type));
@@ -404,16 +398,13 @@ static void pipe_read(void *opaque)
     qemu_mutex_unlock(&card->event_list_mutex);
 }
 
-static int init_pipe_signaling(EmulatedState *card)
+static int init_event_notifier(EmulatedState *card)
 {
-    if (pipe(card->pipe) < 0) {
-        DPRINTF(card, 2, "pipe creation failed\n");
+    if (event_notifier_init(&card->notifier, false) < 0) {
+        DPRINTF(card, 2, "event notifier creation failed\n");
         return -1;
     }
-    fcntl(card->pipe[0], F_SETFL, O_NONBLOCK);
-    fcntl(card->pipe[1], F_SETFL, O_NONBLOCK);
-    fcntl(card->pipe[0], F_SETOWN, getpid());
-    qemu_set_fd_handler(card->pipe[0], pipe_read, NULL, card);
+    event_notifier_set_handler(&card->notifier, card_event_handler);
     return 0;
 }
 
@@ -500,7 +491,7 @@ static int emulated_initfn(CCIDCardState *base)
     qemu_cond_init(&card->handle_apdu_cond);
     card->reader = NULL;
     card->quit_apdu_thread = 0;
-    if (init_pipe_signaling(card) < 0) {
+    if (init_event_notifier(card) < 0) {
         return -1;
     }
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH] ccid-card-emulated: use EventNotifier

[Alon Levy]

> Shut up Coverity's complaint about unchecked fcntl return values,
> and especially make the code simpler and more efficient.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Alon Levy <alevy@redhat.com>

one question below.

> ---
>  hw/usb/ccid-card-emulated.c | 29 ++++++++++-------------------
>  1 file changed, 10 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c
> index 7213c89..aa1c37a 100644
> --- a/hw/usb/ccid-card-emulated.c
> +++ b/hw/usb/ccid-card-emulated.c
> @@ -126,7 +126,7 @@ struct EmulatedState {
>      QemuMutex vreader_mutex; /* and guest_apdu_list mutex */
>      QemuMutex handle_apdu_mutex;
>      QemuCond handle_apdu_cond;
> -    int      pipe[2];
> +    EventNotifier notifier;
>      int      quit_apdu_thread;
>      QemuThread apdu_thread_id;
>  };
> @@ -162,9 +162,7 @@ static void emulated_push_event(EmulatedState *card,
> EmulEvent *event)
>      qemu_mutex_lock(&card->event_list_mutex);
>      QSIMPLEQ_INSERT_TAIL(&(card->event_list), event, entry);
>      qemu_mutex_unlock(&card->event_list_mutex);
> -    if (write(card->pipe[1], card, 1) != 1) {
> -        DPRINTF(card, 1, "write to pipe failed\n");
> -    }
> +    event_notifier_set(&card->notifier);
>  }
>  
>  static void emulated_push_type(EmulatedState *card, uint32_t type)
> @@ -358,16 +356,12 @@ static void *event_thread(void *arg)
>      return NULL;
>  }
>  
> -static void pipe_read(void *opaque)
> +static void card_event_handler(EventNotifier *notifier)
>  {
> -    EmulatedState *card = opaque;
> +    EmulatedState *card = container_of(notifier, EmulatedState, notifier);
>      EmulEvent *event, *next;
> -    char dummy;
> -    int len;
>  
> -    do {
> -        len = read(card->pipe[0], &dummy, sizeof(dummy));
> -    } while (len == sizeof(dummy));
> +    event_notifier_test_and_clear(&card->notifier);

Shouldn't the ignored return value be marked somehow?

>      qemu_mutex_lock(&card->event_list_mutex);
>      QSIMPLEQ_FOREACH_SAFE(event, &card->event_list, entry, next) {
>          DPRINTF(card, 2, "event %s\n",
>          emul_event_to_string(event->p.gen.type));
> @@ -404,16 +398,13 @@ static void pipe_read(void *opaque)
>      qemu_mutex_unlock(&card->event_list_mutex);
>  }
>  
> -static int init_pipe_signaling(EmulatedState *card)
> +static int init_event_notifier(EmulatedState *card)
>  {
> -    if (pipe(card->pipe) < 0) {
> -        DPRINTF(card, 2, "pipe creation failed\n");
> +    if (event_notifier_init(&card->notifier, false) < 0) {
> +        DPRINTF(card, 2, "event notifier creation failed\n");
>          return -1;
>      }
> -    fcntl(card->pipe[0], F_SETFL, O_NONBLOCK);
> -    fcntl(card->pipe[1], F_SETFL, O_NONBLOCK);
> -    fcntl(card->pipe[0], F_SETOWN, getpid());
> -    qemu_set_fd_handler(card->pipe[0], pipe_read, NULL, card);
> +    event_notifier_set_handler(&card->notifier, card_event_handler);
>      return 0;
>  }
>  
> @@ -500,7 +491,7 @@ static int emulated_initfn(CCIDCardState *base)
>      qemu_cond_init(&card->handle_apdu_cond);
>      card->reader = NULL;
>      card->quit_apdu_thread = 0;
> -    if (init_pipe_signaling(card) < 0) {
> +    if (init_event_notifier(card) < 0) {
>          return -1;
>      }
>  
> --
> 1.9.3
> 
> 
> 

Re: [Qemu-devel] [PATCH] ccid-card-emulated: use EventNotifier

[Paolo Bonzini]

Il 23/06/2014 12:38, Alon Levy ha scritto:
>> >  {
>> > -    EmulatedState *card = opaque;
>> > +    EmulatedState *card = container_of(notifier, EmulatedState, notifier);
>> >      EmulEvent *event, *next;
>> > -    char dummy;
>> > -    int len;
>> >
>> > -    do {
>> > -        len = read(card->pipe[0], &dummy, sizeof(dummy));
>> > -    } while (len == sizeof(dummy));
>> > +    event_notifier_test_and_clear(&card->notifier);
> Shouldn't the ignored return value be marked somehow?

It's a pretty common idiom to test-and-clear the event notifier.  Really 
hot paths have a while() loop, but for ccid-card-emulated it's simpler 
to just go through the main loop again.  In fact, it was just what the 
pipe-based code used to do. :)

Paolo