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

[Markus Armbruster <armbru@redhat.com>]

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.

[...]