On Thu, Oct 24, 2019 at 05:09:01AM -0400, Jagannathan Raman wrote:
> diff --git a/hw/proxy/qemu-proxy.c b/hw/proxy/qemu-proxy.c
> index 3b84055..fc1c731 100644
> --- a/hw/proxy/qemu-proxy.c
> +++ b/hw/proxy/qemu-proxy.c
> @@ -337,7 +337,8 @@ static void init_proxy(PCIDevice *dev, char *command, bool need_spawn, Error **e
>  
>      if (!pdev->managed) {
>          if (need_spawn) {
> -            if (!remote_spawn(pdev, command, &local_error)) {
> +            if (remote_spawn(pdev, command, &local_error)) {
> +                fprintf(stderr, "remote spawn failed\n");
>                  return;
>              }
>          }

Looks like a fix for a bug in a previous patch.  Please squash it.
Also, please propagate local_err and do not use fprintf in a function
that has an errp argument for reporting errors.

> +#if defined(CONFIG_MPQEMU)

Maybe these functions should be in a separate file that the makefile
includes when CONFIG_MPQEMU is defined.

> +
> +static PCIProxyDev *get_proxy_object_rid(const char *rid)
> +{
> +    PCIProxyDev *entry;
> +    if (!proxy_list_lock.initialized) {
> +        QLIST_INIT(&proxy_dev_list.devices);
> +        qemu_mutex_init(&proxy_list_lock);
> +    }

This locking approach is broken since exactly-once initialization
semantics are required to avoid races during initialization.  Is the
lock needed at all?

> +DeviceState *qdev_remote_add(QemuOpts *opts, bool device, Error **errp)
> +{
> +    PCIProxyDev *pdev = NULL;
> +    DeviceState *dev;
> +    const char *rid, *rsocket = NULL, *command = NULL;
> +    QDict *qdict_new;
> +    const char *id = NULL;
> +    const char *driver = NULL;
> +    const char *bus = NULL;
> +
> +    if (!proxy_list_lock.initialized) {
> +        QLIST_INIT(&proxy_dev_list.devices);
> +        qemu_mutex_init(&proxy_list_lock);
> +    }
> +
> +    rid = qemu_opt_get(opts, "rid");
> +    if (!rid) {
> +        error_setg(errp, "rdevice option needs rid specified.");
> +        return NULL;
> +    }
> +    if (device) {
> +        driver = qemu_opt_get(opts, "driver");
> +        /* TODO: properly identify the device class. */
> +        if (strncmp(driver, "lsi", 3) == 0) {
> +            id = qemu_opts_id(opts);
> +            if (!id) {
> +                error_setg(errp, "qdev_remote_add option needs id specified.");
> +                return NULL;
> +            }
> +        }
> +    }
> +
> +    rsocket = qemu_opt_get(opts, "socket");
> +    if (rsocket) {
> +        if (strlen(rsocket) > MAX_RID_LENGTH) {
> +            error_setg(errp, "Socket number is incorrect.");
> +            return NULL;
> +        }
> +    }
> +    /*
> +     * TODO: verify command with known commands and on remote end.
> +     * How else can we verify the binary we launch without libvirtd support?
> +     */
> +    command = qemu_opt_get(opts, "command");
> +    if (!rsocket && !command) {
> +        error_setg(errp, "rdevice option needs socket or command specified.");
> +        return NULL;
> +    }
> +
> +    bus = qemu_opt_get(opts, "bus");
> +    dev = qdev_proxy_add(rid, id, (char *)bus, (char *)command,
> +                         rsocket ? atoi(rsocket) : -1,
> +                         rsocket ? true : false, errp);
> +    if (!dev) {
> +        error_setg(errp, "qdev_proxy_add error.");
> +        return NULL;
> +    }
> +
> +    qdict_new = qemu_opts_to_qdict(opts, NULL);
> +
> +    if (!qdict_new) {
> +        error_setg(errp, "Could not parse rdevice options.");
> +        return NULL;
> +    }
> +
> +    pdev = PCI_PROXY_DEV(dev);
> +    if (!pdev->set_remote_opts) {
> +        /* TODO: destroy proxy? */
> +        error_setg(errp, "set_remote_opts failed.");
> +        return NULL;
> +    } else {
> +        if (id && !pdev->dev_id) {
> +            pdev->dev_id = g_strdup(id);
> +        }
> +        pdev->set_remote_opts(PCI_DEVICE(pdev), qdict_new,
> +                              device ? DEV_OPTS : DRIVE_OPTS);

This function needs to be able to return an error if setting the options
failed.  A response message needs to be defined in the protocol to
support this.

Is DRIVE_OPTS still needed?  I thought the drives would be configured in
the remote process and no proxy objects were needed on the QEMU side?