On Thu, Jan 25, 2018 at 1:45 PM, Philippe Mathieu-Daudé
<phili...@mathieu-daude.net> wrote:
> Hi Gerd,
>
> On Thu, Jan 25, 2018 at 6:16 AM, Gerd Hoffmann <kra...@redhat.com> wrote:
>> From: Mao Zhongyi <maozy.f...@cn.fujitsu.com>
>>
>> Replace init() of CCIDCardClass with realize, then convert
>> ccid_card_init(), ccid_card_initfn() and it's callbacks to
>> take an Error** in ordor to report the error more clearly.
>>
>> Cc: Gerd Hoffmann <kra...@redhat.com>
>> Cc: Marc-André Lureau <marcandre.lur...@gmail.com>
>> Cc: Cao jin <caoj.f...@cn.fujitsu.com>
>>
>> Signed-off-by: Mao Zhongyi <maozy.f...@cn.fujitsu.com>
>> Signed-off-by: Cao jin <caoj.f...@cn.fujitsu.com>
>> Message-id:
>> a8b7c670cb61b8096291f5730af62f4230a3e7fa.1514187411.git.maozy.f...@cn.fujitsu.com
>> Signed-off-by: Gerd Hoffmann <kra...@redhat.com>
>> ---
>> hw/usb/ccid.h | 2 +-
>> hw/usb/ccid-card-emulated.c | 44
>> +++++++++++++++++++++----------------------
>> hw/usb/ccid-card-passthru.c | 12 ++++++------
>> hw/usb/dev-smartcard-reader.c | 34 ++++++++++++++++++---------------
>> 4 files changed, 48 insertions(+), 44 deletions(-)
>>
>> diff --git a/hw/usb/ccid.h b/hw/usb/ccid.h
>> index 1f070116d6..6c6c10188d 100644
>> --- a/hw/usb/ccid.h
>> +++ b/hw/usb/ccid.h
>> @@ -34,7 +34,7 @@ typedef struct CCIDCardClass {
>> const uint8_t *apdu,
>> uint32_t len);
>> void (*exitfn)(CCIDCardState *card);
>> - int (*initfn)(CCIDCardState *card);
>> + void (*realize)(CCIDCardState *card, Error **errp);
>> } CCIDCardClass;
>>
>> /*
>> diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c
>> index e646eb243b..daefd9f8f4 100644
>> --- a/hw/usb/ccid-card-emulated.c
>> +++ b/hw/usb/ccid-card-emulated.c
>> @@ -35,6 +35,7 @@
>> #include "qemu/thread.h"
>> #include "qemu/main-loop.h"
>> #include "ccid.h"
>> +#include "qapi/error.h"
>>
>> #define DPRINTF(card, lvl, fmt, ...) \
>> do {\
>> @@ -401,10 +402,10 @@ static void card_event_handler(EventNotifier *notifier)
>> qemu_mutex_unlock(&card->event_list_mutex);
>> }
>>
>> -static int init_event_notifier(EmulatedState *card)
>> +static int init_event_notifier(EmulatedState *card, Error **errp)
>> {
>> if (event_notifier_init(&card->notifier, false) < 0) {
>> - DPRINTF(card, 2, "event notifier creation failed\n");
>> + error_setg(errp, "ccid-card-emul: event notifier creation failed");
>> return -1;
>> }
>> event_notifier_set_handler(&card->notifier, card_event_handler);
>> @@ -480,7 +481,7 @@ static uint32_t parse_enumeration(char *str,
>> return ret;
>> }
>>
>> -static int emulated_initfn(CCIDCardState *base)
>> +static void emulated_realize(CCIDCardState *base, Error **errp)
>> {
>> EmulatedState *card = EMULATED_CCID_CARD(base);
>> VCardEmulError ret;
>> @@ -494,8 +495,8 @@ static int emulated_initfn(CCIDCardState *base)
>> qemu_cond_init(&card->handle_apdu_cond);
>> card->reader = NULL;
>> card->quit_apdu_thread = 0;
>> - if (init_event_notifier(card) < 0) {
>> - return -1;
>> + if (init_event_notifier(card, errp) < 0) {
>> + return;
>> }
>>
>> card->backend = 0;
>> @@ -505,11 +506,11 @@ static int emulated_initfn(CCIDCardState *base)
>> }
>>
>> if (card->backend == 0) {
>> - printf("backend must be one of:\n");
>> + error_setg(errp, "backend must be one of:");
>> for (ptable = backend_enum_table; ptable->name != NULL; ++ptable) {
>> - printf("%s\n", ptable->name);
>> + error_append_hint(errp, "%s\n", ptable->name);
>> }
>> - return -1;
>> + return;
>> }
>>
>> /* TODO: a passthru backened that works on local machine. third card
>> type?*/
>> @@ -517,34 +518,33 @@ static int emulated_initfn(CCIDCardState *base)
>> if (card->cert1 != NULL && card->cert2 != NULL && card->cert3 !=
>> NULL) {
>> ret = emulated_initialize_vcard_from_certificates(card);
>> } else {
>> - printf("%s: you must provide all three certs for"
>> - " certificates backend\n", TYPE_EMULATED_CCID);
>> - return -1;
>> + error_setg(errp, "%s: you must provide all three certs for"
>> + " certificates backend", TYPE_EMULATED_CCID);
>> + return;
>> }
>> } else {
>> if (card->backend != BACKEND_NSS_EMULATED) {
>> - printf("%s: bad backend specified. The options are:\n%s
>> (default),"
>> - " %s.\n", TYPE_EMULATED_CCID, BACKEND_NSS_EMULATED_NAME,
>> - BACKEND_CERTIFICATES_NAME);
>> - return -1;
>> + error_setg(errp, "%s: bad backend specified. The options are:%s"
>> + " (default), %s.", TYPE_EMULATED_CCID,
>> + BACKEND_NSS_EMULATED_NAME,
>> BACKEND_CERTIFICATES_NAME);
>> + return;
>> }
>> if (card->cert1 != NULL || card->cert2 != NULL || card->cert3 !=
>> NULL) {
>> - printf("%s: unexpected cert parameters to nss emulated
>> backend\n",
>> - TYPE_EMULATED_CCID);
>> - return -1;
>> + error_setg(errp, "%s: unexpected cert parameters to nss
>> emulated "
>> + "backend", TYPE_EMULATED_CCID);
>> + return;
>> }
>> /* 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", TYPE_EMULATED_CCID);
>> - return -1;
>> + error_setg(errp, "%s: failed to initialize vcard",
>> TYPE_EMULATED_CCID);
>> + return;
>> }
>> qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread,
>> card, QEMU_THREAD_JOINABLE);
>> qemu_thread_create(&card->apdu_thread_id, "ccid/apdu",
>> handle_apdu_thread,
>> card, QEMU_THREAD_JOINABLE);
>> - return 0;
>> }
>>
>> static void emulated_exitfn(CCIDCardState *base)
>> @@ -581,7 +581,7 @@ static void emulated_class_initfn(ObjectClass *klass,
>> void *data)
>> DeviceClass *dc = DEVICE_CLASS(klass);
>> CCIDCardClass *cc = CCID_CARD_CLASS(klass);
>>
>> - cc->initfn = emulated_initfn;
>> + cc->realize = emulated_realize;
>> cc->exitfn = emulated_exitfn;
>> cc->get_atr = emulated_get_atr;
>> cc->apdu_from_guest = emulated_apdu_from_guest;
>> diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c
>> index 117711862e..b7dd3602dc 100644
>> --- a/hw/usb/ccid-card-passthru.c
>> +++ b/hw/usb/ccid-card-passthru.c
>> @@ -14,6 +14,7 @@
>> #include "qemu/error-report.h"
>> #include "qemu/sockets.h"
>> #include "ccid.h"
>> +#include "qapi/error.h"
>>
>> #define DPRINTF(card, lvl, fmt, ...) \
>> do { \
>> @@ -337,29 +338,28 @@ static const uint8_t *passthru_get_atr(CCIDCardState
>> *base, uint32_t *len)
>> return card->atr;
>> }
>>
>> -static int passthru_initfn(CCIDCardState *base)
>> +static void passthru_realize(CCIDCardState *base, Error **errp)
>> {
>> PassthruState *card = PASSTHRU_CCID_CARD(base);
>>
>> card->vscard_in_pos = 0;
>> card->vscard_in_hdr = 0;
>> if (qemu_chr_fe_backend_connected(&card->cs)) {
>> - DPRINTF(card, D_INFO, "initing chardev\n");
>> + error_setg(errp, "ccid-card-passthru: initing chardev");
>> qemu_chr_fe_set_handlers(&card->cs,
>> ccid_card_vscard_can_read,
>> ccid_card_vscard_read,
>> ccid_card_vscard_event, NULL, card, NULL, true);
>> ccid_card_vscard_send_init(card);
>> } else {
>> - error_report("missing chardev");
>> - return -1;
>> + error_setg(errp, "missing chardev");
>> + return;
>> }
>> card->debug = parse_debug_env("QEMU_CCID_PASSTHRU_DEBUG", D_VERBOSE,
>> card->debug);
>> assert(sizeof(DEFAULT_ATR) <= MAX_ATR_SIZE);
>> memcpy(card->atr, DEFAULT_ATR, sizeof(DEFAULT_ATR));
>> card->atr_length = sizeof(DEFAULT_ATR);
>> - return 0;
>> }
>>
>> static VMStateDescription passthru_vmstate = {
>> @@ -387,7 +387,7 @@ static void passthru_class_initfn(ObjectClass *klass,
>> void *data)
>> DeviceClass *dc = DEVICE_CLASS(klass);
>> CCIDCardClass *cc = CCID_CARD_CLASS(klass);
>>
>> - cc->initfn = passthru_initfn;
>> + cc->realize = passthru_realize;
>> cc->get_atr = passthru_get_atr;
>> cc->apdu_from_guest = passthru_apdu_from_guest;
>> set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
>> diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
>> index e334d3be11..43c564fe7a 100644
>> --- a/hw/usb/dev-smartcard-reader.c
>> +++ b/hw/usb/dev-smartcard-reader.c
>> @@ -510,14 +510,18 @@ static void ccid_card_exitfn(CCIDCardState *card)
>>
>> }
>>
>> -static int ccid_card_initfn(CCIDCardState *card)
>> +static void ccid_card_initfn(CCIDCardState *card, Error **errp)
>> {
>> CCIDCardClass *cc = CCID_CARD_GET_CLASS(card);
>> + Error *local_err = NULL;
>>
>> - if (cc->initfn) {
>> - return cc->initfn(card);
>> + if (cc->realize) {
>> + cc->realize(card, &local_err);
>> + if (local_err != NULL) {
>> + error_propagate(errp, local_err);
>> + return;
>> + }
>> }
>> - return 0;
>> }
>>
>> static bool ccid_has_pending_answers(USBCCIDState *s)
>> @@ -1295,27 +1299,27 @@ static int ccid_card_exit(DeviceState *qdev)
>> return 0;
>> }
>>
>> -static int ccid_card_init(DeviceState *qdev)
>> +static void ccid_card_realize(DeviceState *qdev, Error **errp)
>> {
>> CCIDCardState *card = CCID_CARD(qdev);
>> USBDevice *dev = USB_DEVICE(qdev->parent_bus->parent);
>> USBCCIDState *s = USB_CCID_DEV(dev);
>> - int ret = 0;
>> + Error *local_err = NULL;
>>
>> if (card->slot != 0) {
>> - warn_report("usb-ccid supports one slot, can't add %d",
>> - card->slot);
>> - return -1;
>> + error_setg(errp, "usb-ccid supports one slot, can't add %d",
>> + card->slot);
>> + return;
>> }
>> if (s->card != NULL) {
>> - warn_report("usb-ccid card already full, not adding");
>> - return -1;
>> + error_setg(errp, "usb-ccid card already full, not adding");
>> + return;
>> }
>> - ret = ccid_card_initfn(card);
>> - if (ret == 0) {
>> + ccid_card_initfn(card, &local_err);
>> + if (local_err != NULL) {
>> + error_propagate(errp, local_err);
>
> noticed while rebasing, is this missing?
simpler/cleaner, move out of the if:
+ return;
+ }
+ s->card = card;
- }
>> - return ret;
>> }
>>
>> static void ccid_realize(USBDevice *dev, Error **errp)
>> @@ -1477,7 +1481,7 @@ static void ccid_card_class_init(ObjectClass *klass,
>> void *data)
>> {
>> DeviceClass *k = DEVICE_CLASS(klass);
>> k->bus_type = TYPE_CCID_BUS;
>> - k->init = ccid_card_init;
>> + k->realize = ccid_card_realize;
>> k->exit = ccid_card_exit;
>> k->props = ccid_props;
>> }
>> --
>> 2.9.3
>>
>>
