On 10/26/20 3:47 PM, Greg Kurz wrote:
> On Mon, 26 Oct 2020 14:49:34 +0100
> Philippe Mathieu-Daudé <phi...@redhat.com> wrote:
>
>> On 10/26/20 1:40 PM, Greg Kurz wrote:
>>> spapr_reallocate_hpt() has three users, two of which pass &error_fatal
>>> and the third one, htab_load(), passes &local_err, uses it to detect
>>> failures and simply propagates -EINVAL up to vmstate_load(), which will
>>> cause QEMU to exit. It is thus confusing that spapr_reallocate_hpt()
>>> doesn't return right away when an error is detected in some cases. Also,
>>> the comment suggesting that the caller is welcome to try to carry on
>>> seems like a remnant in this respect.
>>>
>>> This can be improved:
>>> - change spapr_reallocate_hpt() to always report a negative errno on
>>> failure, either as reported by KVM or -ENOSPC if the HPT is smaller
>>> than what was asked,
>>> - use that to detect failures in htab_load() which is preferred over
>>> checking &local_err,
>>> - propagate this negative errno to vmstate_load() because it is more
>>> accurate than propagating -EINVAL for all possible errors.
>>>
>>> Signed-off-by: Greg Kurz <gr...@kaod.org>
...
>>> if (rc < 0) {
>>> @@ -1504,8 +1503,7 @@ void spapr_reallocate_hpt(SpaprMachineState *spapr,
>>> int shift,
>>> error_setg_errno(errp, errno, "Failed to allocate KVM HPT of
>>> order %d",
>>> shift);
>>> error_append_hint(errp, "Try smaller maxmem?\n");
>>> - /* This is almost certainly fatal, but if the caller really
>>> - * wants to carry on with shift == 0, it's welcome to try */
>>> + return -errno;
>>
>> Maybe returning here should be in a previous patch.
>> Otherwise patch looks good.
>>
>
> It could have been indeed...
>
>>> } else if (rc > 0) {
>>> /* kernel-side HPT allocated */
>>> if (rc != shift) {
>>> @@ -1513,6 +1511,7 @@ void spapr_reallocate_hpt(SpaprMachineState *spapr,
>>> int shift,
>>> "Requested order %d HPT, but kernel allocated
>>> order %ld",
>>> shift, rc);
>>> error_append_hint(errp, "Try smaller maxmem?\n");
>>> + return -ENOSPC;
>
> ... along with this one.
>
> I didn't go this way because it doesn't really affect the final behavior since
> QEMU exits in all cases. It's mostly about propagating an appropriate errno up
> to VMState in the case of htab_load(). But if you find it clearer and I need
> to post a v2, I can certainly do that.
As a reviewer I prefer dumb obvious patches I can quickly understand.
If I stop, spend too long, am not sure, I spend time to ask, and usually
stop reviewing the series. Then you spend time to answer, eventually
respin. In doubt, KISS.
Patch is queued so no need for v2.
