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

[Stefan Hajnoczi <stefanha@redhat.com>]

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

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.

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