Re: [PATCH v12 10/19] multi-process: Associate fd of a PCIDevice with its object

["Marc-André Lureau" <marcandre.lureau@gmail.com>]

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

On Mon, Dec 7, 2020 at 6:03 PM Marc-André Lureau <marcandre.lureau@gmail.com>
wrote:

> Hi
>
> On Wed, Dec 2, 2020 at 12:25 AM Jagannathan Raman <jag.raman@oracle.com>
> wrote:
>
>> Associate the file descriptor for a PCIDevice in remote process with
>> DeviceState object.
>>
>> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
>> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
>> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>  include/hw/remote/remote-obj.h |  42 +++++++++++
>>  hw/remote/message.c            |   1 +
>>  hw/remote/remote-obj.c         | 154
>> +++++++++++++++++++++++++++++++++++++++++
>>  MAINTAINERS                    |   2 +
>>  hw/remote/meson.build          |   1 +
>>  5 files changed, 200 insertions(+)
>>  create mode 100644 include/hw/remote/remote-obj.h
>>  create mode 100644 hw/remote/remote-obj.c
>>
>> diff --git a/include/hw/remote/remote-obj.h
>> b/include/hw/remote/remote-obj.h
>> new file mode 100644
>> index 0000000..0e95813
>> --- /dev/null
>> +++ b/include/hw/remote/remote-obj.h
>> @@ -0,0 +1,42 @@
>> +/*
>> + * Copyright © 2020 Oracle and/or its affiliates.
>> + *
>> + * This work is licensed under the terms of the GNU GPL-v2, version 2 or
>> later.
>> + *
>> + * See the COPYING file in the top-level directory.
>> + *
>> + */
>> +
>> +#ifndef REMOTE_OBJECT_H
>> +#define REMOTE_OBJECT_H
>> +
>> +#include "io/channel.h"
>> +#include "qemu/notify.h"
>> +
>> +#define TYPE_REMOTE_OBJECT "x-remote-object"
>> +#define REMOTE_OBJECT(obj) \
>> +    OBJECT_CHECK(RemoteObject, (obj), TYPE_REMOTE_OBJECT)
>> +#define REMOTE_OBJECT_GET_CLASS(obj) \
>> +    OBJECT_GET_CLASS(RemoteObjectClass, (obj), TYPE_REMOTE_OBJECT)
>> +#define REMOTE_OBJECT_CLASS(klass) \
>> +    OBJECT_CLASS_CHECK(RemoteObjectClass, (klass), TYPE_REMOTE_OBJECT)
>> +
>> +typedef struct {
>> +    ObjectClass parent_class;
>> +
>> +    unsigned int nr_devs;
>> +    unsigned int max_devs;
>> +} RemoteObjectClass;
>> +
>> +typedef struct {
>> +    /* private */
>> +    Object parent;
>> +
>> +    Notifier machine_done;
>> +
>> +    int fd;
>> +    char *devid;
>> +    QIOChannel *ioc;
>> +} RemoteObject;
>> +
>> +#endif
>>
>
> There is no need for a header if the header isn't actually shared with
> various .c units. In this series, you can just declare those structs in
> remote-obj.c
>
> diff --git a/hw/remote/message.c b/hw/remote/message.c
>> index 5d87bf4..1f2edc7 100644
>> --- a/hw/remote/message.c
>> +++ b/hw/remote/message.c
>> @@ -56,6 +56,7 @@ void coroutine_fn mpqemu_remote_msg_loop_co(void *data)
>>          }
>>      }
>>      qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
>> +    g_free(com);
>>
>
>
> Should be squashed with the previous patch
>
>
>>      return;
>>  }
>> diff --git a/hw/remote/remote-obj.c b/hw/remote/remote-obj.c
>> new file mode 100644
>> index 0000000..3b4c0d4
>> --- /dev/null
>> +++ b/hw/remote/remote-obj.c
>> @@ -0,0 +1,154 @@
>> +/*
>> + * Copyright © 2020 Oracle and/or its affiliates.
>> + *
>> + * This work is licensed under the terms of the GNU GPL-v2, version 2 or
>> later.
>> + *
>> + * See the COPYING file in the top-level directory.
>> + *
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu-common.h"
>> +
>> +#include "hw/remote/remote-obj.h"
>> +#include "qemu/error-report.h"
>> +#include "qom/object_interfaces.h"
>> +#include "hw/qdev-core.h"
>> +#include "io/channel.h"
>> +#include "hw/qdev-core.h"
>> +#include "hw/remote/machine.h"
>> +#include "io/channel-util.h"
>> +#include "qapi/error.h"
>> +#include "sysemu/sysemu.h"
>> +#include "hw/pci/pci.h"
>> +
>> +static void remote_object_set_fd(Object *obj, const char *str, Error
>> **errp)
>> +{
>> +    RemoteObject *o = REMOTE_OBJECT(obj);
>> +
>> +    o->fd = atoi(str);
>>
>
>  qemu_strtoi() has better error handling semantics. You may also want to
> check it's a valid socket fd with fd_is_socket().
>
> Alternatively, you could use qemu_open() which allows to open from QMP
> fdset ("/dev/fdset/..."). This should be more flexible.
>


A better alternative is qemu_parse_fd().

In some later patch, you use monitor_fd_param(monitor_cur(), ...) for
x-pci-proxy-dev "fd" property.

That might be the right API, for consistency, use it here too?


-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 6261 bytes --]