Re: [PATCH v8 09/20] multi-process: Associate fd of a PCIDevice with its object

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1818 bytes --]

On Fri, Jul 31, 2020 at 02:20:16PM -0400, Jagannathan Raman wrote:
> +static void remote_object_set_devid(Object *obj, const char *str, Error **errp)
> +{
> +    RemoteObject *o = REMOTE_OBJECT(obj);
> +
> +    o->devid = g_strdup(str);

Setter functions can be called multiple times so g_free(o->devid) is
needed to prevent memory leaks.

> +}
> +
> +static void remote_object_machine_done(Notifier *notifier, void *data)
> +{

Is UserCreatableClass->complete() called too early? If you can use it
instead of the machine init done notifier then that would make error
reporting cleaner. Both command-line and object_add monitor commands
would fail as expected if an error occurs, whereas this patch prints an
error and continues running.

> +    RemoteObject *o = container_of(notifier, RemoteObject, machine_done);
> +    DeviceState *dev = NULL;
> +    QIOChannel *ioc = NULL;
> +    Error *err = NULL;
> +
> +    dev = qdev_find_recursive(sysbus_get_default(), o->devid);
> +    if (!dev || !object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> +        error_report("%s is not a PCI device", o->devid);
> +        return;
> +    }

What happens when a "device_del" monitor command removes the device? I
guess a use-after-free will occur since this object isn't aware that the
device disappeared. QOM's Object has a reference count that you can use
to keep the object alive, but that still might not play well with hot
unplug where the device is being removed from its bus. One solution is
to refuse hot unplugging the device while the remote object exists.

> +
> +    ioc = qio_channel_new_fd(o->fd, &err);
> +    if (!ioc) {
> +        error_free(err);
> +        error_report("Failed to allocate IO channel");

Please print err, it contains a more detailed error message than "Failed
to allocate IO channel".

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]