[Qemu-devel] [PATCH 0/2] CCID QOMify

[Qemu-devel] [PATCH 0/2] CCID QOMify

[Cao jin]

As each commit says

Cao jin (2):
  Emulated CCID card: QOMify
  Passthru CCID card: QOMify

 hw/usb/ccid-card-emulated.c | 20 +++++++++-----------
 hw/usb/ccid-card-passthru.c | 10 ++++------
 hw/usb/ccid.h               |  8 ++++++++
 3 files changed, 21 insertions(+), 17 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 1/2] Emulated CCID card: QOMify

[Cao jin]

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"
-
 #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)
-- 
2.1.0

[Qemu-devel] [PATCH 2/2] Passthru CCID card: QOMify

[Cao jin]

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/usb/ccid-card-passthru.c | 10 ++++------
 hw/usb/ccid.h               |  4 ++++
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c
index 9f49c05..cbb7c80 100644
--- a/hw/usb/ccid-card-passthru.c
+++ b/hw/usb/ccid-card-passthru.c
@@ -38,8 +38,6 @@ static const uint8_t DEFAULT_ATR[] = {
  0x13, 0x08
 };
 
-
-#define PASSTHRU_DEV_NAME "ccid-card-passthru"
 #define VSCARD_IN_SIZE 65536
 
 /* maximum size of ATR - from 7816-3 */
@@ -316,7 +314,7 @@ static void ccid_card_vscard_event(void *opaque, int event)
 static void passthru_apdu_from_guest(
     CCIDCardState *base, const uint8_t *apdu, uint32_t len)
 {
-    PassthruState *card = DO_UPCAST(PassthruState, base, base);
+    PassthruState *card = PASSTHRU_CCID_CARD(base);
 
     if (!card->cs) {
         printf("ccid-passthru: no chardev, discarding apdu length %d\n", len);
@@ -327,7 +325,7 @@ static void passthru_apdu_from_guest(
 
 static const uint8_t *passthru_get_atr(CCIDCardState *base, uint32_t *len)
 {
-    PassthruState *card = DO_UPCAST(PassthruState, base, base);
+    PassthruState *card = PASSTHRU_CCID_CARD(base);
 
     *len = card->atr_length;
     return card->atr;
@@ -335,7 +333,7 @@ static const uint8_t *passthru_get_atr(CCIDCardState *base, uint32_t *len)
 
 static int passthru_initfn(CCIDCardState *base)
 {
-    PassthruState *card = DO_UPCAST(PassthruState, base, base);
+    PassthruState *card = PASSTHRU_CCID_CARD(base);
 
     card->vscard_in_pos = 0;
     card->vscard_in_hdr = 0;
@@ -399,7 +397,7 @@ static void passthru_class_initfn(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo passthru_card_info = {
-    .name          = PASSTHRU_DEV_NAME,
+    .name          = TYPE_CCID_PASSTHRU,
     .parent        = TYPE_CCID_CARD,
     .instance_size = sizeof(PassthruState),
     .class_init    = passthru_class_initfn,
diff --git a/hw/usb/ccid.h b/hw/usb/ccid.h
index 315257e..7a3c3f4 100644
--- a/hw/usb/ccid.h
+++ b/hw/usb/ccid.h
@@ -27,6 +27,10 @@ typedef struct CCIDCardInfo CCIDCardInfo;
 #define EMULATED_CCID_CARD(obj) \
      OBJECT_CHECK(EmulatedState, (obj), TYPE_EMULATED_CCID)
 
+#define TYPE_CCID_PASSTHRU "ccid-card-passthru"
+#define PASSTHRU_CCID_CARD(obj) \
+     OBJECT_CHECK(PassthruState, (obj), TYPE_CCID_PASSTHRU)
+
 /*
  * callbacks to be used by the CCID device (hw/usb-ccid.c) to call
  * into the smartcard device (hw/ccid-card-*.c)
-- 
2.1.0

Re: [Qemu-devel] [PATCH 0/2] CCID QOMify

[Cao jin]

Hi mjt,

     I guess these two are suitable for trivial too;) but I forget to cc 
trivial at first...

On 02/01/2016 02:53 PM, Cao jin wrote:
> As each commit says
>
> Cao jin (2):
>    Emulated CCID card: QOMify
>    Passthru CCID card: QOMify
>
>   hw/usb/ccid-card-emulated.c | 20 +++++++++-----------
>   hw/usb/ccid-card-passthru.c | 10 ++++------
>   hw/usb/ccid.h               |  8 ++++++++
>   3 files changed, 21 insertions(+), 17 deletions(-)
>

-- 
Yours Sincerely,

Cao jin

Re: [Qemu-devel] [PATCH 1/2] Emulated CCID card: QOMify

[Michael Tokarev]

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

Re: [Qemu-devel] [PATCH 1/2] Emulated CCID card: QOMify

[Cao jin]

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

-- 
Yours Sincerely,

Cao jin

Re: [Qemu-devel] [PATCH 1/2] Emulated CCID card: QOMify

[Michael Tokarev]

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

Re: [Qemu-devel] [PATCH 1/2] Emulated CCID card: QOMify

[Cao jin]

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

-- 
Yours Sincerely,

Cao jin