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
