Re: [PATCH v13 6/9] vfio/device: Add support for creating dmabuf from multiple ranges

["Cédric Le Goater" <clg@redhat.com>]

On 10/14/26 15:07, Akihiko Odaki wrote:
>>
> 
> Thank you for the guidance. Rephrasing your points:
> 
> - Negative errno is often convenient, but an alternative set of error
>    codes are used when it's a bad fit.
> 
> - KISS: encode specific error conditions to be distinguished instead encoding all.
> 
> This brings us to a common architectural intersection: How should a function handle and propagate errors when its callees return overlapping -errno values that require different treatment by the caller?
> 
> Both virtio_gpu_create_udmabuf() and vfio_device_create_dmabuf_fd() face this challenge. Evaluating the available design options against your advice yields the following:
> 
> 1. Use custom error code enum.
> 
> Defining an enum is not as convenient as negative errno, but it is KISS; it encodes specific error conditions we care about:
> 
> enum {
>      VFIO_DMABUF_CREATE_ERR_INVALID_IOV = -1,
>      VFIO_DMABUF_CREATE_ERR_UNSPEC = -2,
> };
> 
> static int vfio_device_create_dmabuf_fd(..., Error **errp)
> {
>      ...
> 
>      if (!vfio_device_lookup(iov, &vbasedev, &first_rb, errp)) {
>          return VFIO_DMABUF_CREATE_ERR_INVALID_IOV;
>      }
> 
>      ...
> 
>      ret = vfio_device_get_feature(vbasedev, feature);
>      if (ret < 0) {
>          error_setg_errno(errp, -ret,
>                           "Could not create dmabuf fd via VFIO device");
>          return VFIO_DMABUF_CREATE_ERR_UNSPEC;
>      }
>      return ret;
> }

Works, it's heavy.

This function has a single caller, and the enum values break the
-errno convention that the rest of the fallback chain uses
(virtio_gpu_create_udmabuf returns -errno). The caller would
handle two different return-value conventions in the same fallback
sequence.

A VFIO_DMABUF_CREATE_ERR_* enum defined in a VFIO header that
hw/display/virtio-gpu-dmabuf.c must include and switch on.
That's adding a VFIO-specific error vocabulary to the display
subsystem's control flow. It introduces a layering violation.

What is important is to acknowledge that how the implementation
arrives at "not applicable" (device lookup failed, region lookup
failed) is irrelevant to the caller.
A VFIO-specific error enum in the return type inverts thatrelationship;it makes the caller encode knowledge of the implementation's internal
failure modes. That's the opposite of what a library API should do.


> 2. Remap -errno.
> 
> This is what Cédric suggested [1].
> 
> The negative errno values returned by internal callees do not precisely represent the error conditions we need to handle, making negative errno a "bad fit" here. However, the main issue isn't negative errno itself, but rather that we must handle negative errno values originating from different function calls. We can remap the values to resolve this:
> 
> static int vfio_device_create_dmabuf_fd(..., Error **errp)
> {
>      ...
> 
>      if (!vfio_device_lookup(iov, &vbasedev, &first_rb, errp)) {
>          return -EINVAL;
>      }
> 
>      ...
> 
>      ret = vfio_device_get_feature(vbasedev, feature);
>      if (ret < 0) {
>          error_setg_errno(errp, -ret,
>                           "Could not create dmabuf fd via VFIO device");
>          /*
>           * Remap -EINVAL: caller uses it to mean
>           * "addresses incompatible"
>           */
>          return (ret == -EINVAL) ? -EIO : ret;
>      }
>      return ret;
> }
> 
> The downside is that it is not KISS; the condition to remap errno encodes more possible errors, but they do not matter to the current caller (and it coalesces error codes from vfio_device_get_feature(), though that is also harmless for the caller).

The KISS argument is a bit thin here. you acknowledge yourself that neither
concern matters to the caller.

> 3. Another better idea?


If the downside is to encode knowledge about what vfio_device_get_feature()
might return (that the caller should not even know about), and future changes
to that function's error codes could silently break the contract, let's use
-ENODEV as a compromise.

   int vfio_device_create_dmabuf_fd(struct iovec *iov, unsigned int iov_cnt,
                                    Error **errp)
   {
       ...
       if (!vfio_device_lookup(iov, &vbasedev, &first_rb, errp)) {
           return -ENODEV;    <--------
       }
       ...
       for (i = 0; i < iov_cnt; i++) {
           if (!vfio_region_lookup(iov[i].iov_base, &region,
                                   first_rb, &offset, errp)) {
               return -ENODEV; <----------
           }
           ...
       }
       ...
       ret = vfio_device_get_feature(vbasedev, feature);
       if (ret < 0) {
           error_setg_errno(errp, -ret,
                            "Could not create dmabuf fd via VFIO device");
           return ret;   /* pass through kernel errno unchanged */
       }
       return ret;
   }

Caller becomes:

   res->dmabuf_fd = vfio_device_create_dmabuf_fd(res->iov,
                                                 res->iov_cnt,
                                                 &local_err);
   if (res->dmabuf_fd == -ENODEV) {
       error_free_or_abort(&local_err);
       qemu_log_mask(LOG_GUEST_ERROR,
                     "Cannot create dmabuf: incompatible memory\n");
       return false;
   }

Thanks,

C.