diff mbox

[4/5] libcacard: replace qemu thread primitives with glib ones

Message ID 1398751349-20869-5-git-send-email-mjt@msgid.tls.msk.ru
State New
Headers show

Commit Message

Michael Tokarev April 29, 2014, 6:02 a.m. UTC 
Replace QemuMutex with GMutex and QemuCond with GCond
(with corresponding function changes), to make libcacard
independent of qemu internal functions.

Also replace single instance pstrcpy() in vcard_emul_nss.c
to strncpy().  This reverts commit 2e679780ae86c6ca8.

After this step, none of libcacard internals use any
qemu-provided symbols.  Maybe it's a good idea to
stop including qemu-common.h internally too.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 libcacard/Makefile         |    8 +-------
 libcacard/event.c          |   25 ++++++++++++-------------
 libcacard/vcard_emul_nss.c |    3 ++-
 libcacard/vreader.c        |   19 +++++++++----------
 4 files changed, 24 insertions(+), 31 deletions(-)

Comments

Christophe Fergeau April 29, 2014, 8:26 a.m. UTC | #1 
Hey, patch looks good

On Tue, Apr 29, 2014 at 10:02:27AM +0400, Michael Tokarev wrote:
> Replace QemuMutex with GMutex and QemuCond with GCond
> (with corresponding function changes), to make libcacard
> independent of qemu internal functions.
> 
> Also replace single instance pstrcpy() in vcard_emul_nss.c
> to strncpy().  This reverts commit 2e679780ae86c6ca8.

An alternative would be to use g_strlcpy which guarantees
nul-termination.

Christophe
Paolo Bonzini April 29, 2014, 8:38 a.m. UTC | #2 
Il 29/04/2014 10:26, Christophe Fergeau ha scritto:
>> > Replace QemuMutex with GMutex and QemuCond with GCond
>> > (with corresponding function changes), to make libcacard
>> > independent of qemu internal functions.
>> >
>> > Also replace single instance pstrcpy() in vcard_emul_nss.c
>> > to strncpy().  This reverts commit 2e679780ae86c6ca8.
> An alternative would be to use g_strlcpy which guarantees
> nul-termination.

Yes, that is better.

Paolo
Michael Tokarev April 29, 2014, 8:42 a.m. UTC | #3 
29.04.2014 12:38, Paolo Bonzini wrote:
> Il 29/04/2014 10:26, Christophe Fergeau ha scritto:
>>> > Replace QemuMutex with GMutex and QemuCond with GCond
>>> > (with corresponding function changes), to make libcacard
>>> > independent of qemu internal functions.
>>> >
>>> > Also replace single instance pstrcpy() in vcard_emul_nss.c
>>> > to strncpy().  This reverts commit 2e679780ae86c6ca8.
>> An alternative would be to use g_strlcpy which guarantees
>> nul-termination.
> 
> Yes, that is better.

Actually in this very place it isn't really important, given we
always know the exact length of the source and are able to adjust
it to fit into the buffer.  With g_strlcat() code becomes a bit
more ugly... ;)

But mind you, this is the least important change in the whole
patchset.  We are risking to dig into unimportant details and
miss forest for the trees in the result.

Thanks,

/mjt
Paolo Bonzini April 29, 2014, 9:11 a.m. UTC | #4 
Il 29/04/2014 10:42, Michael Tokarev ha scritto:
>>>>> >>> > Also replace single instance pstrcpy() in vcard_emul_nss.c
>>>>> >>> > to strncpy().  This reverts commit 2e679780ae86c6ca8.
>>> >> An alternative would be to use g_strlcpy which guarantees
>>> >> nul-termination.
>> >
>> > Yes, that is better.
> Actually in this very place it isn't really important, given we
> always know the exact length of the source and are able to adjust
> it to fit into the buffer.  With g_strlcat() code becomes a bit
> more ugly... ;)

Uh, now I looked at NEXT_TOKEN and g_strlcpy suddenly becomes less 
palatable.  mempcpy would be nice actually, like

     *mempcpy(dest, src, type_params_length) = 0;

but it is not portable and not wrapped by glib.

Another good alternative is

char *type_str;
...
type_str = g_strndup(type_params, type_params_length);
type = vcard_emul_type_from_string(type_str);
g_free(type_str);

Paolo
Michael Tokarev April 29, 2014, 9:14 a.m. UTC | #5 
29.04.2014 13:11, Paolo Bonzini пишет:
> Il 29/04/2014 10:42, Michael Tokarev ha scritto:
>>>>>> >>> > Also replace single instance pstrcpy() in vcard_emul_nss.c
>>>>>> >>> > to strncpy().  This reverts commit 2e679780ae86c6ca8.
>>>> >> An alternative would be to use g_strlcpy which guarantees
>>>> >> nul-termination.
>>> >
>>> > Yes, that is better.
>> Actually in this very place it isn't really important, given we
>> always know the exact length of the source and are able to adjust
>> it to fit into the buffer.  With g_strlcat() code becomes a bit
>> more ugly... ;)
> 
> Uh, now I looked at NEXT_TOKEN and g_strlcpy suddenly becomes less palatable.

Yess, that's exactly what I mean.

>     mempcpy would be nice actually, like
> 
>     *mempcpy(dest, src, type_params_length) = 0;
> 
> but it is not portable and not wrapped by glib.
> 
> Another good alternative is
> 
> char *type_str;
> ...
> type_str = g_strndup(type_params, type_params_length);
> type = vcard_emul_type_from_string(type_str);
> g_free(type_str);

Actually it is the best one, -- type_str is g_strndup'ed down the line.

I'll do that, in a separate patch before this series.

/mjt
diff mbox

Patch

diff --git a/libcacard/Makefile b/libcacard/Makefile
index 6b06448..89a5942 100644
--- a/libcacard/Makefile
+++ b/libcacard/Makefile
@@ -3,13 +3,7 @@  libcacard_includedir=$(includedir)/cacard
 TOOLS += vscclient$(EXESUF)
 
 # objects linked into a shared library, built with libtool with -fPIC if required
-libcacard-obj-y = $(stub-obj-y) $(libcacard-y)
-libcacard-obj-y += util/osdep.o util/cutils.o util/qemu-timer-common.o
-libcacard-obj-y += util/error.o util/qemu-error.o
-libcacard-obj-$(CONFIG_WIN32) += util/oslib-win32.o util/qemu-thread-win32.o
-libcacard-obj-$(CONFIG_POSIX) += util/oslib-posix.o util/qemu-thread-posix.o
-libcacard-obj-y += $(filter trace/%, $(util-obj-y))
-
+libcacard-obj-y = $(libcacard-y)
 libcacard-lobj-y=$(patsubst %.o,%.lo,$(libcacard-obj-y))
 
 # libtool will build the .o files, too
diff --git a/libcacard/event.c b/libcacard/event.c
index 2d7500f..be11596 100644
--- a/libcacard/event.c
+++ b/libcacard/event.c
@@ -6,7 +6,6 @@ 
  */
 
 #include "qemu-common.h"
-#include "qemu/thread.h"
 
 #include "vcard.h"
 #include "vreader.h"
@@ -43,13 +42,13 @@  vevent_delete(VEvent *vevent)
 
 static VEvent *vevent_queue_head;
 static VEvent *vevent_queue_tail;
-static QemuMutex vevent_queue_lock;
-static QemuCond vevent_queue_condition;
+static GMutex vevent_queue_lock;
+static GCond vevent_queue_condition;
 
 void vevent_queue_init(void)
 {
-    qemu_mutex_init(&vevent_queue_lock);
-    qemu_cond_init(&vevent_queue_condition);
+    g_mutex_init_static(&vevent_queue_lock);
+    g_cond_init_static(&vevent_queue_condition);
     vevent_queue_head = vevent_queue_tail = NULL;
 }
 
@@ -57,7 +56,7 @@  void
 vevent_queue_vevent(VEvent *vevent)
 {
     vevent->next = NULL;
-    qemu_mutex_lock(&vevent_queue_lock);
+    g_mutex_lock(&vevent_queue_lock);
     if (vevent_queue_head) {
         assert(vevent_queue_tail);
         vevent_queue_tail->next = vevent;
@@ -65,8 +64,8 @@  vevent_queue_vevent(VEvent *vevent)
         vevent_queue_head = vevent;
     }
     vevent_queue_tail = vevent;
-    qemu_cond_signal(&vevent_queue_condition);
-    qemu_mutex_unlock(&vevent_queue_lock);
+    g_cond_signal(&vevent_queue_condition);
+    g_mutex_unlock(&vevent_queue_lock);
 }
 
 /* must have lock */
@@ -86,11 +85,11 @@  VEvent *vevent_wait_next_vevent(void)
 {
     VEvent *vevent;
 
-    qemu_mutex_lock(&vevent_queue_lock);
+    g_mutex_lock(&vevent_queue_lock);
     while ((vevent = vevent_dequeue_vevent()) == NULL) {
-        qemu_cond_wait(&vevent_queue_condition, &vevent_queue_lock);
+        g_cond_wait(&vevent_queue_condition, &vevent_queue_lock);
     }
-    qemu_mutex_unlock(&vevent_queue_lock);
+    g_mutex_unlock(&vevent_queue_lock);
     return vevent;
 }
 
@@ -98,9 +97,9 @@  VEvent *vevent_get_next_vevent(void)
 {
     VEvent *vevent;
 
-    qemu_mutex_lock(&vevent_queue_lock);
+    g_mutex_lock(&vevent_queue_lock);
     vevent = vevent_dequeue_vevent();
-    qemu_mutex_unlock(&vevent_queue_lock);
+    g_mutex_unlock(&vevent_queue_lock);
     return vevent;
 }
 
diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
index ee2dfae..0892462 100644
--- a/libcacard/vcard_emul_nss.c
+++ b/libcacard/vcard_emul_nss.c
@@ -1162,7 +1162,8 @@  vcard_emul_options(const char *args)
             NEXT_TOKEN(vname)
             NEXT_TOKEN(type_params)
             type_params_length = MIN(type_params_length, sizeof(type_str)-1);
-            pstrcpy(type_str, type_params_length, type_params);
+            strncpy(type_str, type_params, type_params_length);
+            type_str[type_params_length] = 0;
             type = vcard_emul_type_from_string(type_str);
 
             NEXT_TOKEN(type_params)
diff --git a/libcacard/vreader.c b/libcacard/vreader.c
index 5793d73..91799b4 100644
--- a/libcacard/vreader.c
+++ b/libcacard/vreader.c
@@ -9,10 +9,8 @@ 
 #undef G_LOG_DOMAIN
 #endif
 #define G_LOG_DOMAIN "libcacard"
-#include <glib.h>
 
 #include "qemu-common.h"
-#include "qemu/thread.h"
 
 #include "vcard.h"
 #include "vcard_emul.h"
@@ -28,7 +26,7 @@  struct VReaderStruct {
     VCard *card;
     char *name;
     vreader_id_t id;
-    QemuMutex lock;
+    GMutex lock;
     VReaderEmul  *reader_private;
     VReaderEmulFree reader_private_free;
 };
@@ -97,13 +95,13 @@  apdu_ins_to_string(int ins)
 static inline void
 vreader_lock(VReader *reader)
 {
-    qemu_mutex_lock(&reader->lock);
+    g_mutex_lock(&reader->lock);
 }
 
 static inline void
 vreader_unlock(VReader *reader)
 {
-    qemu_mutex_unlock(&reader->lock);
+    g_mutex_unlock(&reader->lock);
 }
 
 /*
@@ -116,7 +114,7 @@  vreader_new(const char *name, VReaderEmul *private,
     VReader *reader;
 
     reader = (VReader *)g_malloc(sizeof(VReader));
-    qemu_mutex_init(&reader->lock);
+    g_mutex_init(&reader->lock);
     reader->reference_count = 1;
     reader->name = g_strdup(name);
     reader->card = NULL;
@@ -152,6 +150,7 @@  vreader_free(VReader *reader)
         return;
     }
     vreader_unlock(reader);
+    g_mutex_clear(&reader->lock);
     if (reader->card) {
         vcard_free(reader->card);
     }
@@ -413,25 +412,25 @@  vreader_dequeue(VReaderList *list, VReaderListEntry *entry)
 }
 
 static VReaderList *vreader_list;
-static QemuMutex vreader_list_mutex;
+static GMutex vreader_list_mutex;
 
 static void
 vreader_list_init(void)
 {
     vreader_list = vreader_list_new();
-    qemu_mutex_init(&vreader_list_mutex);
+    g_mutex_init_static(&vreader_list_mutex);
 }
 
 static void
 vreader_list_lock(void)
 {
-    qemu_mutex_lock(&vreader_list_mutex);
+    g_mutex_lock(&vreader_list_mutex);
 }
 
 static void
 vreader_list_unlock(void)
 {
-    qemu_mutex_unlock(&vreader_list_mutex);
+    g_mutex_unlock(&vreader_list_mutex);
 }
 
 static VReaderList *