Re: [PATCH v7 09/21] multi-process: Initialize message handler in remote device

[Stefan Hajnoczi]

On Sat, Jun 27, 2020 at 10:09:31AM -0700, elena.ufimtseva@oracle.com wrote:
> diff --git a/hw/i386/remote-msg.c b/hw/i386/remote-msg.c
> new file mode 100644
> index 0000000000..58e24ab2ad
> --- /dev/null
> +++ b/hw/i386/remote-msg.c
> @@ -0,0 +1,52 @@
> +#include "qemu/osdep.h"
> +#include "qemu-common.h"
> +
> +#include "hw/i386/remote.h"
> +#include "io/channel.h"
> +#include "io/mpqemu-link.h"
> +#include "qapi/error.h"
> +#include "sysemu/runstate.h"
> +
> +gboolean mpqemu_process_msg(QIOChannel *ioc, GIOCondition cond,
> +                            gpointer opaque)
> +{
> +    Error *local_err = NULL;
> +    MPQemuMsg msg = { 0 };
> +
> +    if (cond & G_IO_HUP) {
> +        qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> +    }

Missing return FALSE?

> +
> +    if (cond & (G_IO_ERR | G_IO_NVAL)) {
> +        error_setg(&local_err, "Error %d while processing message from proxy 
> \
> +                   in remote process pid=%d", errno, getpid());

This sets and leaks local_err. Should this be error_report()?

> +        return FALSE;
> +    }
> +
> +    if (mpqemu_msg_recv(&msg, ioc) < 0) {
> +        return FALSE;
> +    }
> +
> +    if (!mpqemu_msg_valid(&msg)) {
> +        error_report("Received invalid message from proxy \
> +                     in remote process pid=%d", getpid());
> +        return TRUE;
> +    }

Why return TRUE here but FALSE in previous error cases?

> +
> +    switch (msg.cmd) {
> +    default:
> +        error_setg(&local_err, "Unknown command (%d) received from proxy \
> +                   in remote process pid=%d", msg.cmd, getpid());
> +    }
> +
> +    if (msg.data2) {
> +        free(msg.data2);
> +    }

A mpqemu_msg_cleanup() function in mpqemu-link.h would be nice. That way
the code that frees data2 can be next to the code that allocates it.

Do passed file descriptors need to be closed too (especially in cases
where the command normally does not expect passed file descriptors)?

signature.asc
Description: PGP signature