On 11/20/25 04:29, Akihiko Odaki wrote: > On 2025/11/20 4:16, Dmitry Osipenko wrote: >> On 11/18/25 15:32, Honglei Huang wrote: >>> >>> >>> On 2025/11/18 09:48, Dmitry Osipenko wrote: >>>> On 11/17/25 16:22, Markus Armbruster wrote: >>>>> Dmitry Osipenko <[email protected]> writes: >>>>> >>>>>> On 11/17/25 13:51, Honglei Huang wrote: >>>>>>> diff --git a/hw/display/virtio-gpu-rutabaga.c b/hw/display/virtio- >>>>>>> gpu-rutabaga.c >>>>>>> index ed5ae52acb..ea2928b706 100644 >>>>>>> --- a/hw/display/virtio-gpu-rutabaga.c >>>>>>> +++ b/hw/display/virtio-gpu-rutabaga.c >>>>>>> @@ -466,7 +466,7 @@ rutabaga_cmd_attach_backing(VirtIOGPU *g, >>>>>>> struct virtio_gpu_ctrl_command *cmd) >>>>>>> ret = virtio_gpu_create_mapping_iov(g, att_rb.nr_entries, >>>>>>> sizeof(att_rb), >>>>>>> cmd, NULL, &res->iov, >>>>>>> &res->iov_cnt); >>>>>>> - CHECK(!ret, cmd); >>>>>>> + CHECK(ret >= 0, cmd); >>>>>> >>>>>> virtio_gpu_create_mapping_iov() doesn't return positive values, don't >>>>>> see how this change improves anything. You now saying that ret > 0 is >>>>>> okay, while it shall never happen. >>>>> >>>>> Please see >>>>> >>>>> Subject: Re: [PATCH] virtio-gpu-virgl: fix error handling in >>>>> virgl_cmd_resource_create_blob >>>>> Date: Mon, 17 Nov 2025 08:49:42 +0100 >>>>> Message-ID: <[email protected]> >>>>> https://lore.kernel.org/qemu-devel/[email protected]/ >>>> >>>> It's a rather common bug when errno isn't negated by mistake and a >>>> positive error code is returned. Ignoring positive values when they >>>> aren't expected opens door to unnecessary problems, IMO. >>>> >>> >>> How about apply the v2 or v3 firstly to fix the >>> virtio_gpu_create_mapping_iov() block issue in virtio-gpu? >>> >>> I will create another thread for the `CHECK(!ret, cmd);` thing in >>> rutabaga. >> >> There was a precedent of virtio-gpu not handling positive error codes >> properly [1]. To me there is no problem that needs to be fixed when >> virtio_gpu_create_mapping_iov() is never expected to return positive >> values and doesn't return them. >> >> [1] >> https://lore.kernel.org/qemu-devel/20240129073921.446869-1- >> [email protected]/ >> >> It's a common expectation that errors are negative. But in practice it's >> not always true, especially when interacting with external code. >> >> Functionally this patch doesn't change anything. Will leave to Alex and >> Akihiko to decide on it. > > I'm fine either way. > > This is a "trade-off" scenario where both two options have upsides and > downsides. I often avoid making an argument in that case because it > tends to be subjective and cannot be confirmed, leading to bikeshedding. > > In my opinion, such a function should not return int in the first place, > but should return bool. The wide range of values int can take causes > problems; bool allows compilers to enforce that it only returns two values. > > Sign is one of those problems. int leads to a mistake like returning > errno instead of -errno. > > Functions may have completely different ideas for integer return values. > In case of virtio-gpu, virtio_gpu_create_mapping_iov() returns -1 but > virtio_gpu_update_dmabuf() returns -EINVAL. This is confusing. > > Sometimes it is still preferred to keep functions returning int for > consistency, but looking at include/hw/virtio/virtio-gpu.h, three return > bool and three return int, so returning int here does not improve > consistency. > > That all said, this is a bug fix, not refactoring. This patch does fix a > bug and avoids polluting the codebase, so it is fine for me. Perhaps it > may be nice to have a patch to convert all functions returning int to > bool to avoid pitfalls and improve consistency, but that can be done later.
Let's settle with the current variant, I'm okay with it now. -- Best regards, Dmitry
