Re: [PATCH v6 3/3] vhost-user: add shared_object msg

[Albert Esteve]

On Wed, Sep 6, 2023 at 6:01 PM Albert Esteve <aesteve@redhat.com> wrote:

On Wed, Sep 6, 2023 at 4:43 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

On 6/9/23 16:33, Albert Esteve wrote:

>     I note you  ignored my comment regarding adding a 'Error **' argument in
>     the previous version:
>     911eef0c-d04f-2fcf-e78b-2475cd7af8f0@linaro.org/" rel="noreferrer" target="_blank">https://lore.kernel.org/qemu-devel/911eef0c-d04f-2fcf-e78b-2475cd7af8f0@linaro.org/ <911eef0c-d04f-2fcf-e78b-2475cd7af8f0@linaro.org/" rel="noreferrer" target="_blank">https://lore.kernel.org/qemu-devel/911eef0c-d04f-2fcf-e78b-2475cd7af8f0@linaro.org/>
>
> Sorry I missed those comments somehow :/

Ah, I see.

> I'll check them and resend.

You can also object to them, explaining why this isn't really
useful, if you think so. But first read the big comment in
include/qapi/error.h.

Sure, I understand. So far I tend to trust the judgement of the more experienced

Qemu developers over my own, but if I wouldn't agree with what is suggested I would object :)

So:

- Regarding the two functions with the same, seems to be solved with the squash before,

  and it was probably causing the compile error to begin with, so one less thing to worry about!

- Regarding splitting the commit, sure, no problem. I'll ensure they do compile separately.

- Regarding the error, I read the long comment in the error file and checked surrounding code. I think

  you are right and will be better propagating the error.

But I think I will omit the Error propagation for `vhost_user_backend_handle_shared_object_lookup`.

In this function returning an error code does not necessarily mean that we should log an error.

So if we pass the local_err from the backend_read function to the handler, we cannot be sure

when we should actually print the log.

`vhost_backend_handle_iotlb_msg` has the same issue and does not propagate the error either,

relies solely on `error_report` calls.

Therefore, I will propagate it for `vhost_user_send_resp` and `vhost_user_backend_send_dmabuf_fd` only.

 

And I think I would address all your comments with that! Thanks for the feedback!

 

Thanks,

Phil.