Re: [PATCH 4/4] spapr: Improve spapr_reallocate_hpt() error reporting

[Philippe Mathieu-Daudé]

On 10/26/20 3:47 PM, Greg Kurz wrote:
> On Mon, 26 Oct 2020 14:49:34 +0100
> Philippe Mathieu-Daudé <philmd@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 <groug@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.