| Message ID | 20181225140449.15786-11-fli@suse.com |
|---|---|
| State | New |
| Headers | show |
| Series | [for-4.0,v9,01/16] Fix segmentation fault when qemu_signal_init fails | expand |
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 <armbru@redhat.com> > Cc: David Gibson <david@gibson.dropbear.id.au> > Signed-off-by: Fei Li <fli@suse.com> This looks like a good change, but it no longer applies due to a change in the qemu_thread_create() signature. > --- > 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. > + } > > spapr->pending_hpt = pending; >
在 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 <armbru@redhat.com> >> Cc: David Gibson <david@gibson.dropbear.id.au> >> Signed-off-by: Fei Li <fli@suse.com> > 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? > >> --- >> 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; >>
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 <armbru@redhat.com> > > > Cc: David Gibson <david@gibson.dropbear.id.au> > > > Signed-off-by: Fei Li <fli@suse.com> > > 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. > > > > > --- > > > 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; >
在 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 <armbru@redhat.com> >>>> Cc: David Gibson <david@gibson.dropbear.id.au> >>>> Signed-off-by: Fei Li <fli@suse.com> >>> 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;
On Thu, Jan 03, 2019 at 09:41:49PM +0800, Fei Li wrote: > > 在 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 <armbru@redhat.com> > > > > > Cc: David Gibson <david@gibson.dropbear.id.au> > > > > > Signed-off-by: Fei Li <fli@suse.com> > > > > 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 :) Ah, sorry. Since I was only CCed on this patch, not the rest of the series, I assumed it was independent and didn't think to check the earlier patches of the series. So, yes, I think the global change I'm referring to is 6/16, which I didn't have, so that explains the problem. In that case it's probably best if this goes in via the same tree the rest of the series is going to. So, with the H_HARDWARE change made: Acked-by: David Gibson <david@gibson.dropbear.id.au> > 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; >
在 2019/1/4 下午1:21, David Gibson 写道: > On Thu, Jan 03, 2019 at 09:41:49PM +0800, Fei Li wrote: >> 在 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 <armbru@redhat.com> >>>>>> Cc: David Gibson <david@gibson.dropbear.id.au> >>>>>> Signed-off-by: Fei Li <fli@suse.com> >>>>> 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 :) > Ah, sorry. Since I was only CCed on this patch, not the rest of the > series, I assumed it was independent and didn't think to check the > earlier patches of the series. A good reminder, CCing all during the review stage seems more reasonable. :) > > So, yes, I think the global change I'm referring to is 6/16, which I > didn't have, so that explains the problem. > > In that case it's probably best if this goes in via the same tree the > rest of the series is going to. So, with the H_HARDWARE change made: > > Acked-by: David Gibson <david@gibson.dropbear.id.au> Thanks for the review! Have a nice day ;) Fei > > >> 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;
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; + } spapr->pending_hpt = pending;
Add a local_err to hold the error, and return the corresponding error code to replace the temporary &error_abort. Cc: Markus Armbruster <armbru@redhat.com> Cc: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Fei Li <fli@suse.com> --- hw/ppc/spapr_hcall.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)