> On Oct 27, 2021, at 11:59 AM, Stefan Hajnoczi <stefa...@redhat.com> wrote:
>
> On Mon, Oct 11, 2021 at 01:31:09AM -0400, Jagannathan Raman wrote:
>> @@ -94,9 +101,31 @@ static void vfu_object_set_device(Object *obj, const
>> char *str, Error **errp)
>> trace_vfu_prop("device", str);
>> }
>>
>> +/*
>> + * vfio-user-server depends on the availability of the 'socket' and 'device'
>> + * properties. It also depends on devices instantiated in QEMU. These
>> + * dependencies are not available during the instance_init phase of this
>> + * object's life-cycle. As such, the server is initialized after the
>> + * machine is setup. machine_init_done_notifier notifies vfio-user-server
>> + * when the machine is setup, and the dependencies are available.
>> + */
>> +static void vfu_object_machine_done(Notifier *notifier, void *data)
>> +{
>> + VfuObject *o = container_of(notifier, VfuObject, machine_done);
>
> Was there a check for non-NULL o->socket before this? Maybe it's not
> needed because QAPI treats 'socket' as a required field and refuses to
> create the SocketAddress if it's missing?
Yes, “socket” is a required option. The server will not launch without that
option.
I believe optional parameters are defined within ‘[‘ and ‘]’ braces in
"./qapi/qom.json"
>
>> +
>> + o->vfu_ctx = vfu_create_ctx(VFU_TRANS_SOCK, o->socket->u.q_unix.path, 0,
>> + o, VFU_DEV_TYPE_PCI);
>> + if (o->vfu_ctx == NULL) {
>> + error_setg(&error_abort, "vfu: Failed to create context - %s",
>> + strerror(errno));
>
> The error reporting needs to be synchronous so that hotplugging with
> object-add fails instead of succeeding and leaving a failed object.
OK, will make the change for the error reporting to be synchronous.
>
> In the startup case (not hotplug) it's okay to abort.
>
>> + return;
>> + }
>> +}
>> +
>> static void vfu_object_init(Object *obj)
>> {
>> VfuObjectClass *k = VFU_OBJECT_GET_CLASS(obj);
>> + VfuObject *o = VFU_OBJECT(obj);
>>
>> if (!object_dynamic_cast(OBJECT(current_machine), TYPE_REMOTE_MACHINE)) {
>> error_setg(&error_abort, "vfu: %s only compatible with %s machine",
>> @@ -111,7 +140,12 @@ static void vfu_object_init(Object *obj)
>> return;
>> }
>>
>> + o->vfu_ctx = NULL;
>
> The object's fields are initialized to 0 so this isn't necessary.
Got it.
>
>> +
>> k->nr_devs++;
>> +
>> + o->machine_done.notify = vfu_object_machine_done;
>> + qemu_add_machine_init_done_notifier(&o->machine_done);
>
> The notifier is invoked immediately if the machine has already been
> initialized. That means vfu_object_machine_done() is called before the
> properties ('socket' and 'device') have been set when object-add hotplug
> is used. I think this needs to be moved elsewhere.
We’ll check this scenario out. Thank you!
>> }
>>
>> static void vfu_object_finalize(Object *obj)
>> @@ -123,6 +157,10 @@ static void vfu_object_finalize(Object *obj)
>>
>> g_free(o->socket);
>>
>> + if (o->vfu_ctx) {
>> + vfu_destroy_ctx(o->vfu_ctx);
>> + }
>> +
>> g_free(o->device);
>>
>> if (k->nr_devs == 0) {
>
> Missing qemu_remove_machine_init_done_notifier().
Got it.
