Re: [RFC v4 PATCH 07/49] multi-process: define mpqemu-link object

[Jag Raman <jag.raman@oracle.com>]

On 11/13/2019 10:53 AM, Stefan Hajnoczi wrote:
> On Thu, Oct 24, 2019 at 05:08:48AM -0400, Jagannathan Raman wrote:
>> +#ifndef MPQEMU_LINK_H
>> +#define MPQEMU_LINK_H
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu-common.h"
>> +
>> +#include <stddef.h>
>> +#include <stdint.h>
> 
> These are already included by "qemu/osdep.h".
> 
>> +#include <pthread.h>
> 
> Is <pthread.h> needed?

It's not needed. We'll remove it.

> 
>> +
>> +#include "qom/object.h"
>> +#include "qemu/thread.h"
>> +
>> +#define TYPE_MPQEMU_LINK "mpqemu-link"
>> +#define MPQEMU_LINK(obj) \
>> +    OBJECT_CHECK(MPQemuLinkState, (obj), TYPE_MPQEMU_LINK)
>> +
>> +#define REMOTE_MAX_FDS 8
>> +
>> +#define MPQEMU_MSG_HDR_SIZE offsetof(MPQemuMsg, data1.u64)
>> +
>> +/**
>> + * mpqemu_cmd_t:
>> + * CONF_READ        PCI config. space read
>> + * CONF_WRITE       PCI config. space write
>> + *
>> + * proc_cmd_t enum type to specify the command to be executed on the remote
>> + * device.
>> + */
>> +typedef enum {
>> +    INIT = 0,
>> +    CONF_READ,
>> +    CONF_WRITE,
>> +    MAX,
>> +} mpqemu_cmd_t;
> 
> Please allow for future non-PCI devices by clearly naming PCI-specific
> commands and including a bus type in the initialization messages.

OK, will do.

> 
>> diff --git a/io/mpqemu-link.c b/io/mpqemu-link.c
>> new file mode 100644
>> index 0000000..b39f4d0
>> --- /dev/null
>> +++ b/io/mpqemu-link.c
>> @@ -0,0 +1,309 @@
>> +/*
>> + * Communication channel between QEMU and remote device process
>> + *
>> + * Copyright 2019, Oracle and/or its affiliates.
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>> + * of this software and associated documentation files (the "Software"), to deal
>> + * in the Software without restriction, including without limitation the rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu-common.h"
>> +
>> +#include <assert.h>
>> +#include <errno.h>
>> +#include <pthread.h>
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <sys/types.h>
>> +#include <sys/socket.h>
>> +#include <sys/un.h>
>> +#include <unistd.h>
>> +#include <limits.h>
>> +#include <poll.h>
> 
> Many of these are already included by "qemu/osdep.h".  Some of them
> shouldn't be used directly because QEMU or glib have abstractions that
> hide the platform-specific differences (e.g. pthread, poll).
> 
>> +MPQemuLinkState *mpqemu_link_create(void)
>> +{
>> +    return MPQEMU_LINK(object_new(TYPE_MPQEMU_LINK));
>> +}
> 
> I'm not sure what the purpose of this object is.  mpqemu_link_create()
> suggests the objects will be created internally instead of via -object
> mpqemu-link,..., which is unusual.
> 
> mpqemu_msg_send() and mpqemu_msg_recv() seem to be the main functions
> but they do not even use their MPQemuLinkState *s argument.

The LINK object is made up of multiple CHANNELS objects. For example, a
link between QEMU & the remote process could be comprised of multiple
channels.

You're correct, mpqemu_msg_send() & mpqemu_msg_recv() don't use the the
argument "s". This was a consequence of adding the multi-channel
support, before which this argument was used. We will fix this in the
next review.

Thank you!
--
Jag

> 
>> +void mpqemu_start_coms(MPQemuLinkState *s)
>> +{
>> +
>> +    g_assert(g_source_attach(&s->com->gsrc, s->ctx));
>> +
>> +    g_main_loop_run(s->loop);
>> +}
> 
> There is already IOThread if you need an event loop thread.  But does
> this need to be its own thread?  The communication should be
> asynchronous and therefore it can run in the main event loop or any
> existing IOThread.
>