diff mbox

[1/2] Emulated CCID card: QOMify

Message ID 1454309636-18263-2-git-send-email-caoj.fnst@cn.fujitsu.com
State New
Headers show

Commit Message

Cao jin Feb. 1, 2016, 6:53 a.m. UTC 
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/usb/ccid-card-emulated.c | 20 +++++++++-----------
 hw/usb/ccid.h               |  4 ++++
 2 files changed, 13 insertions(+), 11 deletions(-)

Comments

Michael Tokarev Feb. 2, 2016, 8:15 a.m. UTC | #1 
01.02.2016 09:53, Cao jin wrote:
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/usb/ccid-card-emulated.c | 20 +++++++++-----------
>  hw/usb/ccid.h               |  4 ++++
>  2 files changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c
> index 869a63c..0b05260 100644
> --- a/hw/usb/ccid-card-emulated.c
> +++ b/hw/usb/ccid-card-emulated.c
> @@ -42,8 +42,6 @@ do {\
>      } \
>  } while (0)
>  
> -#define EMULATED_DEV_NAME "ccid-card-emulated"


> --- a/hw/usb/ccid.h
> +++ b/hw/usb/ccid.h
> +#define TYPE_EMULATED_CCID "ccid-card-emulated"
> +#define EMULATED_CCID_CARD(obj) \
> +     OBJECT_CHECK(EmulatedState, (obj), TYPE_EMULATED_CCID)

Why did you move the type definition from .c to .h file?
It is only referenced in .c, no?

Ditto for the second patch in this series.

Thanks,

/mjt
Cao jin Feb. 2, 2016, 8:25 a.m. UTC | #2 
On 02/02/2016 04:15 PM, Michael Tokarev wrote:
> 01.02.2016 09:53, Cao jin wrote:
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>>   hw/usb/ccid-card-emulated.c | 20 +++++++++-----------
>>   hw/usb/ccid.h               |  4 ++++
>>   2 files changed, 13 insertions(+), 11 deletions(-)
>>
>> diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c
>> index 869a63c..0b05260 100644
>> --- a/hw/usb/ccid-card-emulated.c
>> +++ b/hw/usb/ccid-card-emulated.c
>> @@ -42,8 +42,6 @@ do {\
>>       } \
>>   } while (0)
>>
>> -#define EMULATED_DEV_NAME "ccid-card-emulated"
>
>
>> --- a/hw/usb/ccid.h
>> +++ b/hw/usb/ccid.h
>> +#define TYPE_EMULATED_CCID "ccid-card-emulated"
>> +#define EMULATED_CCID_CARD(obj) \
>> +     OBJECT_CHECK(EmulatedState, (obj), TYPE_EMULATED_CCID)
>
> Why did you move the type definition from .c to .h file?
> It is only referenced in .c, no?
>
> Ditto for the second patch in this series.
>
> Thanks,
>
> /mjt
>

Yes, that type definition is referred only in .c for now, but we are not 
sure about it in the future, for better flexibility, I think put it into 
.h is better. http://wiki.qemu.org/QOMConventions also says:

DO use TYPE_FOO constants, defined in a header if used in other parts of 
code
Michael Tokarev Feb. 2, 2016, 8:30 a.m. UTC | #3 
02.02.2016 11:25, Cao jin wrote:
> On 02/02/2016 04:15 PM, Michael Tokarev wrote:
>> Why did you move the type definition from .c to .h file?
>> It is only referenced in .c, no?
[]
> Yes, that type definition is referred only in .c for now, but we are not sure about it in the future, for better flexibility, I think put it into .h is better. http://wiki.qemu.org/QOMConventions also says:
> 
> DO use TYPE_FOO constants, defined in a header if used in other parts of code

The last words in this sentense makes sense, I think:
"if used in other parts of the code".

So I read it as 2-part suggestion:

1) use TYPE_foo constants instead of "literals", and
2) define them in a header file if used elsewhere too.

The "future flexibility" is, in my opinion, questionable.
If we'll use them in the future, we can always move the
#define to the header file.  For now, there's no need
to clutter the heade more, but there's a good reason
to use the #define in the "home" .c file where this
device is implemented, in all places where the device
name is referenced.

At least that's how I read this (somewhat ambiguous)
suggestion in the wiki.

Thanks,

/mjt
Cao jin Feb. 2, 2016, 9 a.m. UTC | #4 
On 02/02/2016 04:30 PM, Michael Tokarev wrote:
> 02.02.2016 11:25, Cao jin wrote:
>> On 02/02/2016 04:15 PM, Michael Tokarev wrote:
>>> Why did you move the type definition from .c to .h file?
>>> It is only referenced in .c, no?
> []
>> Yes, that type definition is referred only in .c for now, but we are not sure about it in the future, for better flexibility, I think put it into .h is better. http://wiki.qemu.org/QOMConventions also says:
>>
>> DO use TYPE_FOO constants, defined in a header if used in other parts of code
>
> The last words in this sentense makes sense, I think:
> "if used in other parts of the code".
>
> So I read it as 2-part suggestion:
>
> 1) use TYPE_foo constants instead of "literals", and
> 2) define them in a header file if used elsewhere too.
>
> The "future flexibility" is, in my opinion, questionable.
> If we'll use them in the future, we can always move the
> #define to the header file.  For now, there's no need
> to clutter the heade more, but there's a good reason
> to use the #define in the "home" .c file where this
> device is implemented, in all places where the device
> name is referenced.
>
> At least that's how I read this (somewhat ambiguous)
> suggestion in the wiki.
>
> Thanks,
>
> /mjt
>

leave it where it was is also fine by me, it is not late to move it to 
header file when it is certainly to be used in other parts. I will 
update it:)
diff mbox

Patch

diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c
index 869a63c..0b05260 100644
--- a/hw/usb/ccid-card-emulated.c
+++ b/hw/usb/ccid-card-emulated.c
@@ -42,8 +42,6 @@  do {\
     } \
 } while (0)
 
-#define EMULATED_DEV_NAME "ccid-card-emulated"
-
 #define BACKEND_NSS_EMULATED_NAME "nss-emulated"
 #define BACKEND_CERTIFICATES_NAME "certificates"
 
@@ -133,7 +131,7 @@  struct EmulatedState {
 static void emulated_apdu_from_guest(CCIDCardState *base,
     const uint8_t *apdu, uint32_t len)
 {
-    EmulatedState *card = DO_UPCAST(EmulatedState, base, base);
+    EmulatedState *card = EMULATED_CCID_CARD(base);
     EmulEvent *event = (EmulEvent *)g_malloc(sizeof(EmulEvent) + len);
 
     assert(event);
@@ -150,7 +148,7 @@  static void emulated_apdu_from_guest(CCIDCardState *base,
 
 static const uint8_t *emulated_get_atr(CCIDCardState *base, uint32_t *len)
 {
-    EmulatedState *card = DO_UPCAST(EmulatedState, base, base);
+    EmulatedState *card = EMULATED_CCID_CARD(base);
 
     *len = card->atr_length;
     return card->atr;
@@ -478,7 +476,7 @@  static uint32_t parse_enumeration(char *str,
 
 static int emulated_initfn(CCIDCardState *base)
 {
-    EmulatedState *card = DO_UPCAST(EmulatedState, base, base);
+    EmulatedState *card = EMULATED_CCID_CARD(base);
     VCardEmulError ret;
     const EnumTable *ptable;
 
@@ -514,26 +512,26 @@  static int emulated_initfn(CCIDCardState *base)
             ret = emulated_initialize_vcard_from_certificates(card);
         } else {
             printf("%s: you must provide all three certs for"
-                   " certificates backend\n", EMULATED_DEV_NAME);
+                   " certificates backend\n", TYPE_EMULATED_CCID);
             return -1;
         }
     } else {
         if (card->backend != BACKEND_NSS_EMULATED) {
             printf("%s: bad backend specified. The options are:\n%s (default),"
-                " %s.\n", EMULATED_DEV_NAME, BACKEND_NSS_EMULATED_NAME,
+                " %s.\n", TYPE_EMULATED_CCID, BACKEND_NSS_EMULATED_NAME,
                 BACKEND_CERTIFICATES_NAME);
             return -1;
         }
         if (card->cert1 != NULL || card->cert2 != NULL || card->cert3 != NULL) {
             printf("%s: unexpected cert parameters to nss emulated backend\n",
-                   EMULATED_DEV_NAME);
+                   TYPE_EMULATED_CCID);
             return -1;
         }
         /* default to mirroring the local hardware readers */
         ret = wrap_vcard_emul_init(NULL);
     }
     if (ret != VCARD_EMUL_OK) {
-        printf("%s: failed to initialize vcard\n", EMULATED_DEV_NAME);
+        printf("%s: failed to initialize vcard\n", TYPE_EMULATED_CCID);
         return -1;
     }
     qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread,
@@ -545,7 +543,7 @@  static int emulated_initfn(CCIDCardState *base)
 
 static int emulated_exitfn(CCIDCardState *base)
 {
-    EmulatedState *card = DO_UPCAST(EmulatedState, base, base);
+    EmulatedState *card = EMULATED_CCID_CARD(base);
     VEvent *vevent = vevent_new(VEVENT_LAST, NULL, NULL);
 
     vevent_queue_vevent(vevent); /* stop vevent thread */
@@ -588,7 +586,7 @@  static void emulated_class_initfn(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo emulated_card_info = {
-    .name          = EMULATED_DEV_NAME,
+    .name          = TYPE_EMULATED_CCID,
     .parent        = TYPE_CCID_CARD,
     .instance_size = sizeof(EmulatedState),
     .class_init    = emulated_class_initfn,
diff --git a/hw/usb/ccid.h b/hw/usb/ccid.h
index 9334da8..315257e 100644
--- a/hw/usb/ccid.h
+++ b/hw/usb/ccid.h
@@ -23,6 +23,10 @@  typedef struct CCIDCardInfo CCIDCardInfo;
 #define CCID_CARD_GET_CLASS(obj) \
      OBJECT_GET_CLASS(CCIDCardClass, (obj), TYPE_CCID_CARD)
 
+#define TYPE_EMULATED_CCID "ccid-card-emulated"
+#define EMULATED_CCID_CARD(obj) \
+     OBJECT_CHECK(EmulatedState, (obj), TYPE_EMULATED_CCID)
+
 /*
  * callbacks to be used by the CCID device (hw/usb-ccid.c) to call
  * into the smartcard device (hw/ccid-card-*.c)