Jag Raman <jag.ra...@oracle.com> writes:
>> On May 5, 2022, at 10:42 AM, Markus Armbruster <arm...@redhat.com> wrote:
>>
>> Jag Raman <jag.ra...@oracle.com> writes:
>>
>>>> On May 5, 2022, at 3:44 AM, Markus Armbruster <arm...@redhat.com> wrote:
>>>>
>>>> Jag Raman <jag.ra...@oracle.com> writes:
>>>>
>>>>>> On May 4, 2022, at 7:42 AM, Markus Armbruster <arm...@redhat.com> wrote:
>>>>>>
>>>>>> Jagannathan Raman <jag.ra...@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.ufimts...@oracle.com>
>>>>>>> Signed-off-by: John G Johnson <john.g.john...@oracle.com>
>>>>>>> Signed-off-by: Jagannathan Raman <jag.ra...@oracle.com>
>>>>>>> Reviewed-by: Stefan Hajnoczi <stefa...@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.
[...]
