Re: [PATCH v6 1/2] s390x/pci: Add PCI error handling for vfio pci devices

[Matthew Rosato <mjrosato@linux.ibm.com>]

On 9/30/26 1:46 PM, Farhan Ali wrote:
> 
> On 9/30/2026 9:30 AM, Matthew Rosato wrote:
>>> +static int s390_pci_get_feature_err(VFIOPCIDevice *vfio_pci,
>>> +                                    PciCcdfErr *ccdf,
>>> +                                    uint32_t ccdf_err_length,
>>> +                                    Error **errp)
>>> +{
>>> +    ERRP_GUARD();
>>> +    int ret;
>>> +    size_t total_size;
>>> +    struct vfio_device_feature_zpci_err *err;
>>> +    g_autofree void *buf = NULL;
>>> +    g_autofree struct vfio_device_feature *feature = NULL;
>>> +
>>> +    total_size = sizeof(*feature) + sizeof(*err);
>>> +    feature = g_malloc(total_size);
>>> +    feature->argsz = total_size;
>>> +    feature->flags = VFIO_DEVICE_FEATURE_GET |
>>> VFIO_DEVICE_FEATURE_ZPCI_ERROR;
>>> +
>>> +    buf = g_malloc(ccdf_err_length);
>>> +    err = (void *)feature->data;
>>> +    err->data = (uint64_t)buf;
>>> +    ret = vfio_device_get_feature(&vfio_pci->vbasedev, feature);
>>> +
>>> +    if (ret) {
>>> +        error_setg(errp, "Failed feature get
>>> VFIO_DEVICE_FEATURE_ZPCI_ERROR"
>>> +                   " (rc=%d)", ret);
>> I see this was changed from last version, but doesn't this fall under
>> 'avoid useless error object creation and destruction' described in
>> qapi/error.h for the -ENOMSG return value?
>>
>> We create it here only to explicitly destroy it from the caller.
>>
>> Can we instead switch to the negative/non-negative return structure with
>> something like...
>>
>> <0: error as defined (so we set errp)
>> 0-N: number of errors to process (and we do not set errp)
>>
>> For the non-negative case we can realistically only have 0 (-ENOMSG maps
>> to this) or 1 (the feature found something - it is reporting 1 error to
>> handle).
> 
> I was trying to follow what I assumed to be existing pattern for
> handling expected errno[1]. I am okay to change it, but I would like to
> understand what is the preferred approach here.
> 
> [1] https://elixir.bootlin.com/qemu/v11.1.2/source/hw/vfio/iommufd.c#L408
> 
AFAICT the example you provided is using a shared routine where in some
cases the errp generated for -EINVAL is actually propagated vs just
thrown out immediately. (see line 483 of the same reference)

In your case, this isn't a shared routine and the errp associated with
-ENOMSG will never be used.

But anyway, I think we're at a point of semantics.

You made this change based on [a] from include/qapi/error.h:
 * - On success, the function should not touch *errp.

And you've done that successfully.

But I'm complaining about [b] from the same file:
 * - Whenever practical, also return a value that indicates success /
 *   failure.  This can make the error checking more concise, and can
 *   avoid useless error object creation and destruction.

AFAIU, the point is to always have an errp when there was a failure, and
to not have an errp when there is a success.

Do you consider the -ENOMSG return from the ioctl a success or a
failure?  It seems like a success to me (the ioctl worked, there was
just nothing to get), but you're treating it as an error case.  That
satisfies [a] but I'm suggesting it violates [b] since you create an
errp that is guaranteed to never be used.

So as for preferred approach for this: I would prefer not to create
Error objects that are guaranteed not to get used, but I will also not
reject the series on that alone.

Thanks,
Matt