Re: [PATCH v9 10/17] vfio-user: run vfio-user context

[Jag Raman]

> On May 6, 2022, at 1:44 AM, Markus Armbruster <armbru@redhat.com> wrote:
> 
> Jag Raman <jag.raman@oracle.com> writes:
> 
>>> On May 5, 2022, at 10:42 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>> 
>>> Jag Raman <jag.raman@oracle.com> writes:
>>> 
>>>>> On May 5, 2022, at 3:44 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>>>> 
>>>>> Jag Raman <jag.raman@oracle.com> writes:
>>>>> 
>>>>>>> On May 4, 2022, at 7:42 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>>>>>> 
>>>>>>> Jagannathan Raman <jag.raman@oracle.com> writes:
>>>>>>> 
>>>>>>>> Setup a handler to run vfio-user context. The context is driven by
>>>>>>>> messages to the file descriptor associated with it - get the fd for
>>>>>>>> the context and hook up the handler with it
>>>>>>>> 
>>>>>>>> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
>>>>>>>> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
>>>>>>>> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
>>>>>>>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> 
>>> [...]
>>> 
>>>>>>>> @@ -164,6 +172,76 @@ static void vfu_object_set_device(Object *obj, 
>>>>>>>> const char *str, Error **errp)
>>>>>>>> vfu_object_init_ctx(o, errp);
>>>>>>>> }
>>>>>>>> 
>>>>>>>> +static void vfu_object_ctx_run(void *opaque)
>>>>>>>> +{
>>>>>>>> + VfuObject *o = opaque;
>>>>>>>> + const char *vfu_id;
>>>>>>>> + char *vfu_path, *pci_dev_path;
>>>>>>>> + int ret = -1;
>>>>>>>> +
>>>>>>>> + while (ret != 0) {
>>>>>>>> + ret = vfu_run_ctx(o->vfu_ctx);
>>>>>>>> + if (ret < 0) {
>>>>>>>> + if (errno == EINTR) {
>>>>>>>> + continue;
>>>>>>>> + } else if (errno == ENOTCONN) {
>>>>>>>> + vfu_id = object_get_canonical_path_component(OBJECT(o));
>>>>>>>> + vfu_path = object_get_canonical_path(OBJECT(o));
>>>>>>> 
>>>>>>> Hmm. @vfu_id is always the last component of @vfu_path. Why do we need
>>>>>>> to send both?
>>>>>> 
>>>>>> vfu_id is the ID that the user/Orchestrator passed as a command-line 
>>>>>> option
>>>>>> during addition/creation. So it made sense to report back with the same 
>>>>>> ID
>>>>>> that they used. But I’m OK with dropping this if that’s what you prefer.
>>>>> 
>>>>> Matter of taste, I guess.  I'd drop it simply to saves us the trouble of
>>>>> documenting it.
>>>>> 
>>>>> If we decide to keep it, then I think we should document it's always the
>>>>> last component of @vfu_path.
>>>>> 
>>>>>>>> + g_assert(o->pci_dev);
>>>>>>>> + pci_dev_path = object_get_canonical_path(OBJECT(o->pci_dev));
>>>>>>>> + qapi_event_send_vfu_client_hangup(vfu_id, vfu_path,
>>>>>>>> + o->device, pci_dev_path);
>>>>>>> 
>>>>>>> Where is o->device set? I'm asking because I it must not be null here,
>>>>>>> and that's not locally obvious.
>>>>>> 
>>>>>> Yeah, it’s not obvious from this patch that o->device is guaranteed to be
>>>>>> non-NULL. It’s set by vfu_object_set_device(). Please see the following
>>>>>> patches in the series:
>>>>>> vfio-user: define vfio-user-server object
>>>>>> vfio-user: instantiate vfio-user context
>>>>> 
>>>>> vfu_object_set_device() is a QOM property setter.  It gets called if and
>>>>> only if the property is set.  If it's never set, ->device remains null.
>>>>> What ensures it's always set?
>>>> 
>>>> That’s a good question - it’s not obvious from this patch.
>>>> 
>>>> The code would not reach here if o->device is not set. If o->device is 
>>>> NULL,
>>>> vfu_object_init_ctx() would bail out early without setting up
>>>> vfu_object_attach_ctx() and vfu_object_ctx_run() (this function)
>>>> handlers.
>>> 
>>> Yes:
>>> 
>>>   static void vfu_object_init_ctx(VfuObject *o, Error **errp)
>>>   {
>>>       ERRP_GUARD();
>>>       DeviceState *dev = NULL;
>>>       vfu_pci_type_t pci_type = VFU_PCI_TYPE_CONVENTIONAL;
>>>       int ret;
>>> 
>>>       if (o->vfu_ctx || !o->socket || !o->device ||
>>>               !phase_check(PHASE_MACHINE_READY)) {
>>>           return;
>>>       }
>>> 
>>> Bails out without setting an error.  Sure that's appropriate?
>> 
>> It’s not an error per se - vfu_object_init_ctx() doesn’t proceed
>> further if device/socket is not set or if the machine is not ready.
>> 
>> Both socket and device are required properties, so they would
>> eventually be set by vfu_object_set_socket() /
>> vfu_object_set_device() - and these setters eventually kick
>> vfu_object_init_ctx().
> 
> Early returns from a function that sets error on failure triggers bug
> spider sense, because forgetting to set an error on failure is a fairly
> common mistake.
> 
> The root of the problem is of course that the function's contract is not
> obvious.  The name vfu_object_init_ctx() suggests it initializes
> something.  But it clearly doesn't unless certain conditions are met.
> The reader is left to wonder whether that's a bug, or whether that's
> what it is supposed to do.
> 
> A function contract spelling out when the function is supposed to do
> what (including "nothing") would help.

Sure, will add a comment explaining what this function is
supposed to do.

--
Jag

> 
> [...]
>