> On May 6, 2022, at 1:44 AM, Markus Armbruster <arm...@redhat.com> wrote:
>
> 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.
Sure, will add a comment explaining what this function is
supposed to do.
--
Jag
>
> [...]
>
