| Message ID | 20190201051806.53183-7-lifei1214@126.com |
|---|---|
| State | New |
| Headers | show |
| Series | qemu_thread_create: propagate the error to callers to handle | expand |
Fei Li <lifei1214@126.com> writes: > From: Fei Li <fli@suse.com> > > Utilize the existed errp to propagate the error and do the > corresponding cleanup to replace the temporary &error_abort. > > Cc: Markus Armbruster <armbru@redhat.com> > Cc: Gerd Hoffmann <kraxel@redhat.com> > Signed-off-by: Fei Li <fli@suse.com> > --- > hw/usb/ccid-card-emulated.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c > index 0b170f6328..19b4b9a8fa 100644 > --- a/hw/usb/ccid-card-emulated.c > +++ b/hw/usb/ccid-card-emulated.c > @@ -544,11 +544,16 @@ static void emulated_realize(CCIDCardState *base, Error **errp) > error_setg(errp, "%s: failed to initialize vcard", TYPE_EMULATED_CCID); > goto out2; > } > - /* TODO: let the further caller handle the error instead of abort() here */ > - qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread, > - card, QEMU_THREAD_JOINABLE, &error_abort); > - qemu_thread_create(&card->apdu_thread_id, "ccid/apdu", handle_apdu_thread, > - card, QEMU_THREAD_JOINABLE, &error_abort); > + if (qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread, > + card, QEMU_THREAD_JOINABLE, errp) < 0) { > + goto out2; > + } > + if (qemu_thread_create(&card->apdu_thread_id, "ccid/apdu", > + handle_apdu_thread, card, > + QEMU_THREAD_JOINABLE, errp) < 0) { > + qemu_thread_join(&card->event_thread_id); What makes event_thread terminate here? I'm asking because if it doesn't, the join will hang. > + goto out2; > + } > > return;
在 2019/2/1 下午9:04, Markus Armbruster 写道: > Fei Li <lifei1214@126.com> writes: > >> From: Fei Li <fli@suse.com> >> >> Utilize the existed errp to propagate the error and do the >> corresponding cleanup to replace the temporary &error_abort. >> >> Cc: Markus Armbruster <armbru@redhat.com> >> Cc: Gerd Hoffmann <kraxel@redhat.com> >> Signed-off-by: Fei Li <fli@suse.com> >> --- >> hw/usb/ccid-card-emulated.c | 15 ++++++++++----- >> 1 file changed, 10 insertions(+), 5 deletions(-) >> >> diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c >> index 0b170f6328..19b4b9a8fa 100644 >> --- a/hw/usb/ccid-card-emulated.c >> +++ b/hw/usb/ccid-card-emulated.c >> @@ -544,11 +544,16 @@ static void emulated_realize(CCIDCardState *base, Error **errp) >> error_setg(errp, "%s: failed to initialize vcard", TYPE_EMULATED_CCID); >> goto out2; >> } >> - /* TODO: let the further caller handle the error instead of abort() here */ >> - qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread, >> - card, QEMU_THREAD_JOINABLE, &error_abort); >> - qemu_thread_create(&card->apdu_thread_id, "ccid/apdu", handle_apdu_thread, >> - card, QEMU_THREAD_JOINABLE, &error_abort); >> + if (qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread, >> + card, QEMU_THREAD_JOINABLE, errp) < 0) { >> + goto out2; >> + } >> + if (qemu_thread_create(&card->apdu_thread_id, "ccid/apdu", >> + handle_apdu_thread, card, >> + QEMU_THREAD_JOINABLE, errp) < 0) { >> + qemu_thread_join(&card->event_thread_id); > What makes event_thread terminate here? > > I'm asking because if it doesn't, the join will hang. Oops, my neglect.. Could we add a `qemu_thread_cancel(card->event_thread_id);` here before the join()? Have a nice day, thanks Fei > >> + goto out2; >> + } >> >> return;
Fei Li <lifei1214@126.com> writes: > 在 2019/2/1 下午9:04, Markus Armbruster 写道: >> Fei Li <lifei1214@126.com> writes: >> >>> From: Fei Li <fli@suse.com> >>> >>> Utilize the existed errp to propagate the error and do the >>> corresponding cleanup to replace the temporary &error_abort. >>> >>> Cc: Markus Armbruster <armbru@redhat.com> >>> Cc: Gerd Hoffmann <kraxel@redhat.com> >>> Signed-off-by: Fei Li <fli@suse.com> >>> --- >>> hw/usb/ccid-card-emulated.c | 15 ++++++++++----- >>> 1 file changed, 10 insertions(+), 5 deletions(-) >>> >>> diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c >>> index 0b170f6328..19b4b9a8fa 100644 >>> --- a/hw/usb/ccid-card-emulated.c >>> +++ b/hw/usb/ccid-card-emulated.c >>> @@ -544,11 +544,16 @@ static void emulated_realize(CCIDCardState *base, Error **errp) >>> error_setg(errp, "%s: failed to initialize vcard", TYPE_EMULATED_CCID); >>> goto out2; >>> } >>> - /* TODO: let the further caller handle the error instead of abort() here */ >>> - qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread, >>> - card, QEMU_THREAD_JOINABLE, &error_abort); >>> - qemu_thread_create(&card->apdu_thread_id, "ccid/apdu", handle_apdu_thread, >>> - card, QEMU_THREAD_JOINABLE, &error_abort); >>> + if (qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread, >>> + card, QEMU_THREAD_JOINABLE, errp) < 0) { >>> + goto out2; >>> + } >>> + if (qemu_thread_create(&card->apdu_thread_id, "ccid/apdu", >>> + handle_apdu_thread, card, >>> + QEMU_THREAD_JOINABLE, errp) < 0) { >>> + qemu_thread_join(&card->event_thread_id); >> What makes event_thread terminate here? >> >> I'm asking because if it doesn't, the join will hang. > Oops, my neglect.. Could we add a > `qemu_thread_cancel(card->event_thread_id);` here > before the join()? pthread_cancel() is difficult to use correctly, and we don't use it in QEMU so far. Instead, we tell threads to stop, e.g. by setting a flag the thread checks in its main loop, and making sure the thread actually loops in bounded time. How to best achieve that for this thread I don't know. Christophe, Marc-André, can you help?
在 2019/2/4 下午9:30, Markus Armbruster 写道: > Fei Li <lifei1214@126.com> writes: > >> 在 2019/2/1 下午9:04, Markus Armbruster 写道: >>> Fei Li <lifei1214@126.com> writes: >>> >>>> From: Fei Li <fli@suse.com> >>>> >>>> Utilize the existed errp to propagate the error and do the >>>> corresponding cleanup to replace the temporary &error_abort. >>>> >>>> Cc: Markus Armbruster <armbru@redhat.com> >>>> Cc: Gerd Hoffmann <kraxel@redhat.com> >>>> Signed-off-by: Fei Li <fli@suse.com> >>>> --- >>>> hw/usb/ccid-card-emulated.c | 15 ++++++++++----- >>>> 1 file changed, 10 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c >>>> index 0b170f6328..19b4b9a8fa 100644 >>>> --- a/hw/usb/ccid-card-emulated.c >>>> +++ b/hw/usb/ccid-card-emulated.c >>>> @@ -544,11 +544,16 @@ static void emulated_realize(CCIDCardState *base, Error **errp) >>>> error_setg(errp, "%s: failed to initialize vcard", TYPE_EMULATED_CCID); >>>> goto out2; >>>> } >>>> - /* TODO: let the further caller handle the error instead of abort() here */ >>>> - qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread, >>>> - card, QEMU_THREAD_JOINABLE, &error_abort); >>>> - qemu_thread_create(&card->apdu_thread_id, "ccid/apdu", handle_apdu_thread, >>>> - card, QEMU_THREAD_JOINABLE, &error_abort); >>>> + if (qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread, >>>> + card, QEMU_THREAD_JOINABLE, errp) < 0) { >>>> + goto out2; >>>> + } >>>> + if (qemu_thread_create(&card->apdu_thread_id, "ccid/apdu", >>>> + handle_apdu_thread, card, >>>> + QEMU_THREAD_JOINABLE, errp) < 0) { >>>> + qemu_thread_join(&card->event_thread_id); >>> What makes event_thread terminate here? >>> >>> I'm asking because if it doesn't, the join will hang. >> Oops, my neglect.. Could we add a >> `qemu_thread_cancel(card->event_thread_id);` here >> before the join()? > pthread_cancel() is difficult to use correctly, and we don't use it in > QEMU so far. Instead, we tell threads to stop, e.g. by setting a flag > the thread checks in its main loop, and making sure the thread actually > loops in bounded time. How to best achieve that for this thread I don't > know. Christophe, Marc-André, can you help? Hi, Christophe, Marc-André, Would you like to share your views and give some suggestions? :) That would be very helpful, thanks a lot! Have a nice day Fei
在 2019/2/15 下午8:35, Fei Li 写道: > > 在 2019/2/4 下午9:30, Markus Armbruster 写道: >> Fei Li <lifei1214@126.com> writes: >> >>> 在 2019/2/1 下午9:04, Markus Armbruster 写道: >>>> Fei Li <lifei1214@126.com> writes: >>>> >>>>> From: Fei Li <fli@suse.com> >>>>> >>>>> Utilize the existed errp to propagate the error and do the >>>>> corresponding cleanup to replace the temporary &error_abort. >>>>> >>>>> Cc: Markus Armbruster <armbru@redhat.com> >>>>> Cc: Gerd Hoffmann <kraxel@redhat.com> >>>>> Signed-off-by: Fei Li <fli@suse.com> >>>>> --- >>>>> hw/usb/ccid-card-emulated.c | 15 ++++++++++----- >>>>> 1 file changed, 10 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/hw/usb/ccid-card-emulated.c >>>>> b/hw/usb/ccid-card-emulated.c >>>>> index 0b170f6328..19b4b9a8fa 100644 >>>>> --- a/hw/usb/ccid-card-emulated.c >>>>> +++ b/hw/usb/ccid-card-emulated.c >>>>> @@ -544,11 +544,16 @@ static void emulated_realize(CCIDCardState >>>>> *base, Error **errp) >>>>> error_setg(errp, "%s: failed to initialize vcard", >>>>> TYPE_EMULATED_CCID); >>>>> goto out2; >>>>> } >>>>> - /* TODO: let the further caller handle the error instead of >>>>> abort() here */ >>>>> - qemu_thread_create(&card->event_thread_id, "ccid/event", >>>>> event_thread, >>>>> - card, QEMU_THREAD_JOINABLE, &error_abort); >>>>> - qemu_thread_create(&card->apdu_thread_id, "ccid/apdu", >>>>> handle_apdu_thread, >>>>> - card, QEMU_THREAD_JOINABLE, &error_abort); >>>>> + if (qemu_thread_create(&card->event_thread_id, "ccid/event", >>>>> event_thread, >>>>> + card, QEMU_THREAD_JOINABLE, errp) < 0) { >>>>> + goto out2; >>>>> + } >>>>> + if (qemu_thread_create(&card->apdu_thread_id, "ccid/apdu", >>>>> + handle_apdu_thread, card, >>>>> + QEMU_THREAD_JOINABLE, errp) < 0) { >>>>> + qemu_thread_join(&card->event_thread_id); >>>> What makes event_thread terminate here? >>>> >>>> I'm asking because if it doesn't, the join will hang. >>> Oops, my neglect.. Could we add a >>> `qemu_thread_cancel(card->event_thread_id);` here >>> before the join()? >> pthread_cancel() is difficult to use correctly, and we don't use it in >> QEMU so far. Instead, we tell threads to stop, e.g. by setting a flag >> the thread checks in its main loop, and making sure the thread actually >> loops in bounded time. How to best achieve that for this thread I don't >> know. Christophe, Marc-André, can you help? > Hi, Christophe, Marc-André, > Would you like to share your views and give some suggestions? :) > That would be very helpful, thanks a lot! > > Have a nice day > Fei > Hi all, I refer to the method in emulated_unrealize(): terminate event_thread by stopping vevent thread [1]. But I am not sure whether this is proper, please share your views in [PATCH v12 for-4.1 06/11]. Thanks a lot! :) [1] VEvent *vevent = vevent_new(VEVENT_LAST, NULL, NULL); vevent_queue_vevent(vevent); /* stop vevent thread */ qemu_thread_join(&card->event_thread_id); Have a nice day, thanks Fei
diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c index 0b170f6328..19b4b9a8fa 100644 --- a/hw/usb/ccid-card-emulated.c +++ b/hw/usb/ccid-card-emulated.c @@ -544,11 +544,16 @@ static void emulated_realize(CCIDCardState *base, Error **errp) error_setg(errp, "%s: failed to initialize vcard", TYPE_EMULATED_CCID); goto out2; } - /* TODO: let the further caller handle the error instead of abort() here */ - qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread, - card, QEMU_THREAD_JOINABLE, &error_abort); - qemu_thread_create(&card->apdu_thread_id, "ccid/apdu", handle_apdu_thread, - card, QEMU_THREAD_JOINABLE, &error_abort); + if (qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread, + card, QEMU_THREAD_JOINABLE, errp) < 0) { + goto out2; + } + if (qemu_thread_create(&card->apdu_thread_id, "ccid/apdu", + handle_apdu_thread, card, + QEMU_THREAD_JOINABLE, errp) < 0) { + qemu_thread_join(&card->event_thread_id); + goto out2; + } return;