David Gibson <dgib...@redhat.com> writes: > On Thu, 13 Dec 2018 08:26:48 +0100 > Markus Armbruster <arm...@redhat.com> wrote: > >> There's a question for David Gibson inline. Please search for /ppc/. >> >> Fei Li <f...@suse.com> writes: >> >> > Make qemu_thread_create() return a Boolean to indicate if it succeeds >> > rather than failing with an error. And add an Error parameter to hold >> > the error message and let the callers handle it. >> >> The "rather than failing with an error" is misleading. Before the >> patch, we report to stderr and abort(). What about: >> >> qemu-thread: Make qemu_thread_create() handle errors properly >> >> qemu_thread_create() abort()s on error. Not nice. Give it a >> return value and an Error ** argument, so it can return success / >> failure. >> >> Still missing from the commit message then: how you update the callers. >> Let's see below. > > [snip] >> > --- 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,8 +539,13 @@ static target_ulong h_resize_hpt_prepare(PowerPCCPU >> > *cpu, >> > pending->shift = shift; >> > pending->ret = H_HARDWARE; >> > >> > - qemu_thread_create(&pending->thread, "sPAPR HPT prepare", >> > - hpt_prepare_thread, pending, QEMU_THREAD_DETACHED); >> > + 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; >> > + } >> > >> > spapr->pending_hpt = pending; >> > >> >> This is a caller that returns an error code on failure. You change it >> to report the error, then return failure. The return failure part looks >> fine. Whether reporting the error is appropriate I can't say for sure. >> No other failure mode reports anything. David, what do you think? > > I think it's reasonable here. In this context error returns and > reported errors are for different audiences. The error returns are for > the guest, the reported errors are for the guest administrator or > management layers. This particularly failure is essentially a host > side fault that is mostly relevant to the VM management. We have to > say *something* to the guest to explain that the action couldn't go > forward and H_RESOURCE makes as much sense as anything.
Double-checking: is it okay to report some failures of this function (one of two H_RESOURCE failures, to be precise), but not others?