> 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().
>
>> Also, device is a required parameter. QEMU would not initialize this object
>> without it. Please see the definition of VfioUserServerProperties in the
>> following patch - noting that optional parameters are prefixed with a ‘*’:
>> [PATCH v9 07/17] vfio-user: define vfio-user-server object.
>>
>> May be we should add a comment here to explain why o->device
>> wouldn’t be NULL?
>
> Perhaps assertion with a comment explaining why it holds.
OK, will do.
--
Jag
>
>> Thank you!
>
> You're welcome!
>
