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, ®ion,
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.
