Re: [PATCH v7 11/21] multi-process: introduce proxy object

[Stefan Hajnoczi]

On Sat, Jun 27, 2020 at 10:09:33AM -0700, elena.ufimtseva@oracle.com wrote:
> From: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> 
> Defines a PCI Device proxy object as a child of TYPE_PCI_DEVICE.
> 
> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
> ---
>  MAINTAINERS            |  2 ++
>  hw/pci/Makefile.objs   |  1 +
>  hw/pci/proxy.c         | 70 ++++++++++++++++++++++++++++++++++++++++++
>  include/hw/pci/proxy.h | 43 ++++++++++++++++++++++++++
>  4 files changed, 116 insertions(+)
>  create mode 100644 hw/pci/proxy.c
>  create mode 100644 include/hw/pci/proxy.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 017c96eace..b48c3114c1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2952,6 +2952,8 @@ F: include/io/mpqemu-link.h
>  F: hw/i386/remote-msg.c
>  F: include/hw/i386/remote-memory.h
>  F: hw/i386/remote-memory.c
> +F: hw/pci/proxy.c
> +F: include/hw/pci/proxy.h
>  
>  Build and test automation
>  -------------------------
> diff --git a/hw/pci/Makefile.objs b/hw/pci/Makefile.objs
> index c78f2fb24b..515dda506c 100644
> --- a/hw/pci/Makefile.objs
> +++ b/hw/pci/Makefile.objs
> @@ -12,3 +12,4 @@ common-obj-$(CONFIG_PCI_EXPRESS) += pcie_port.o pcie_host.o
>  
>  common-obj-$(call lnot,$(CONFIG_PCI)) += pci-stub.o
>  common-obj-$(CONFIG_ALL) += pci-stub.o
> +obj-$(CONFIG_MPQEMU) += proxy.o
> diff --git a/hw/pci/proxy.c b/hw/pci/proxy.c
> new file mode 100644
> index 0000000000..6d62399c52
> --- /dev/null
> +++ b/hw/pci/proxy.c
> @@ -0,0 +1,70 @@
> +/*
> + * Copyright © 2018, 2020 Oracle and/or its affiliates.
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu-common.h"
> +
> +#include "hw/pci/proxy.h"
> +#include "hw/pci/pci.h"
> +#include "qapi/error.h"
> +#include "io/channel-util.h"
> +#include "hw/qdev-properties.h"
> +#include "monitor/monitor.h"
> +
> +static void proxy_set_socket(PCIProxyDev *pdev, int fd, Error **errp)
> +{
> +    pdev->com = qio_channel_new_fd(fd, errp);

The caller needs to close(fd) on failure:

  if (!pdev->com) {
      close(fd);
  }

> +}

pdev->com is never freed. It seems like hotplug should be possible
eventually. Implementing ->unrealize() from the start will make that
easier because it will be necessary to think through the lifecycle of
this object.

> +
> +static Property proxy_properties[] = {
> +    DEFINE_PROP_STRING("fd", PCIProxyDev, fd),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void pci_proxy_dev_realize(PCIDevice *device, Error **errp)
> +{
> +    PCIProxyDev *dev = PCI_PROXY_DEV(device);
> +    int proxyfd;
> +
> +    if (dev->fd) {

Does it make sense to succeed without fd? If not then errp should be
set so the caller knows .realize() failed.

> +        proxyfd = monitor_fd_param(cur_mon, dev->fd, errp);
> +        if (proxyfd == -1) {
> +            error_prepend(errp, "proxy: unable to parse proxyfd: ");

The user-visible property name is "fd":
s/proxyfd/fd/

> +typedef struct PCIProxyDevClass {
> +    PCIDeviceClass parent_class;
> +
> +    void (*realize)(PCIProxyDev *dev, Error **errp);
> +
> +    char *command;
> +} PCIProxyDevClass;

Neither ->realize nor ->command are used in this patch. Please drop
PCIProxyDevClass for now. If these fields are used later on then they
should be introduced in the patches that need them.

signature.asc
Description: PGP signature