From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:53471) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gf3GM-0002dX-QI for qemu-devel@nongnu.org; Thu, 03 Jan 2019 08:42:19 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gf3GE-0002iu-3L for qemu-devel@nongnu.org; Thu, 03 Jan 2019 08:42:17 -0500 Received: from m50-112.126.com ([123.125.50.112]:60796) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gf3GD-0002gD-Cg for qemu-devel@nongnu.org; Thu, 03 Jan 2019 08:42:10 -0500 References: <20181225140449.15786-1-fli@suse.com> <20181225140449.15786-11-fli@suse.com> <20190102023609.GC27457@umbus.fritz.box> <2c10669c-2f43-6b98-ef02-c4b2edbd21ad@126.com> <20190103034305.GQ10853@umbus.fritz.box> From: Fei Li Message-ID: <76ea4ac2-e60f-b762-a365-5924445ccd6c@126.com> Date: Thu, 3 Jan 2019 21:41:49 +0800 MIME-Version: 1.0 In-Reply-To: <20190103034305.GQ10853@umbus.fritz.box> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH for-4.0 v9 10/16] qemu_thread: supplement error handling for h_resize_hpt_prepare List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: Fei Li , qemu-devel@nongnu.org, shirley17fei@gmail.com, Markus Armbruster 在 2019/1/3 上午11:43, David Gibson 写道: > On Wed, Jan 02, 2019 at 02:44:17PM +0800, 李菲 wrote: >> 在 2019/1/2 上午10:36, David Gibson 写道: >>> On Tue, Dec 25, 2018 at 10:04:43PM +0800, Fei Li wrote: >>>> Add a local_err to hold the error, and return the corresponding >>>> error code to replace the temporary &error_abort. >>>> >>>> Cc: Markus Armbruster >>>> Cc: David Gibson >>>> Signed-off-by: Fei Li >>> This looks like a good change, but it no longer applies due to a >>> change in the qemu_thread_create() signature. >> Sorry that I am not sure whether I understand. Do you mean using >> &error_abort is more suitable for this handling, rather than report >> the &local_err & return a failure reason? > No, I just mean that context has been altered by a global change and > the patch will need to be fixed up to cope with that. Just to be clearer: does the "global change" mean the "[patch 06/16] qemu_thread: Make qemu_thread_create() handle errors properly", or another patch not in this patch series? If it means the [patch 06/16], I want to explain more: the 06/16 handles all qemu_thread_create() by passing &error_abort as the parameter, and the following patches are to improve on the &error_abort for callers who can handle more properly. E.g. if qemu_thread_create() fails in h_resize_hpt_prepare(), I think reporting the &local_err & returning the failure reason is more proper than just abort() inside qemu_thread_create() when calls error_setg_errno(). In other words, this patch is actually written to apply to patch 06. And I have no clue where it needs to be fixed up. Please correct me if I understand wrong. Have a nice day, thanks :) Fei > >>>> --- >>>> hw/ppc/spapr_hcall.c | 12 ++++++++---- >>>> 1 file changed, 8 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c >>>> index 5bc2cf4540..7c16ade04a 100644 >>>> --- a/hw/ppc/spapr_hcall.c >>>> +++ b/hw/ppc/spapr_hcall.c >>>> @@ -478,6 +478,7 @@ static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu, >>>> sPAPRPendingHPT *pending = spapr->pending_hpt; >>>> uint64_t current_ram_size; >>>> int rc; >>>> + Error *local_err = NULL; >>>> if (spapr->resize_hpt == SPAPR_RESIZE_HPT_DISABLED) { >>>> return H_AUTHORITY; >>>> @@ -538,10 +539,13 @@ static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu, >>>> pending->shift = shift; >>>> pending->ret = H_HARDWARE; >>>> - /* TODO: let the further caller handle the error instead of abort() here */ >>>> - qemu_thread_create(&pending->thread, "sPAPR HPT prepare", >>>> - hpt_prepare_thread, pending, >>>> - QEMU_THREAD_DETACHED, &error_abort); >>>> + if (!qemu_thread_create(&pending->thread, "sPAPR HPT prepare", >>>> + hpt_prepare_thread, pending, >>>> + QEMU_THREAD_DETACHED, &local_err)) { >>>> + error_reportf_err(local_err, "failed to create hpt_prepare_thread: "); >>>> + g_free(pending); >>>> + return H_RESOURCE; >>> I also think H_HARDWARE would be a better choice here. Although the >>> failure is due to a resource constraint, it's not because the guest >>> asked for too much, just because the host is in dire straits. From >>> the guest's point of view it's basically a hardware failure. >> Ok, thanks. Will use H_HARDWARE instead. >> >> Have a nice day, thanks for the review. :) >> Fei >>>> + } >>>> spapr->pending_hpt = pending;