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

Commit Message

Elena Ufimtseva June 27, 2020, 5:09 p.m. UTC

From: Jagannathan Raman <[e-mail]>

Initializes the message handler function in the remote process. It is
called whenever there's an event pending on QIOChannel that registers
this function.

Signed-off-by: Elena Ufimtseva <[e-mail]>
Signed-off-by: John G Johnson <[e-mail]>
Signed-off-by: Jagannathan Raman <[e-mail]>
---
 MAINTAINERS              |  1 +
 hw/i386/Makefile.objs    |  1 +
 hw/i386/remote-msg.c     | 52 ++++++++++++++++++++++++++++++++++++++++
 hw/i386/remote.c         |  4 ++++
 include/hw/i386/remote.h |  3 +++
 5 files changed, 61 insertions(+)
 create mode 100644 hw/i386/remote-msg.c

Comments

Stefan Hajnoczi July 1, 2020, 6:53 a.m. UTC | #1

On Sat, Jun 27, 2020 at 10:09:31AM -0700, [e-mail] 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)?

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 50a5fc53d6..e204b3e0c6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2949,6 +2949,7 @@  F: hw/i386/remote.c
 F: include/hw/i386/remote.h
 F: io/mpqemu-link.c
 F: include/io/mpqemu-link.h
+F: hw/i386/remote-msg.c
 
 Build and test automation
 -------------------------
diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
index 5ead266a15..83969585c1 100644
--- a/hw/i386/Makefile.objs
+++ b/hw/i386/Makefile.objs
@@ -15,6 +15,7 @@  obj-$(CONFIG_VMPORT) += vmport.o
 obj-$(CONFIG_VMMOUSE) += vmmouse.o
 obj-$(CONFIG_PC) += port92.o
 obj-$(CONFIG_MPQEMU) += remote.o
+obj-$(CONFIG_MPQEMU) += remote-msg.o
 
 obj-y += kvmvapic.o
 obj-$(CONFIG_ACPI) += acpi-common.o
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);
+    }
+
+    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());
+        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;
+    }
+
+    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);
+    }
+
+    if (local_err) {
+        error_report_err(local_err);
+        return FALSE;
+    }
+
+    return TRUE;
+}
diff --git a/hw/i386/remote.c b/hw/i386/remote.c
index 1a1becffe0..5342e884ad 100644
--- a/hw/i386/remote.c
+++ b/hw/i386/remote.c
@@ -16,6 +16,7 @@ 
 #include "exec/memory.h"
 #include "qapi/error.h"
 #include "io/channel-util.h"
+#include "io/channel.h"
 
 static void remote_machine_init(MachineState *machine)
 {
@@ -50,6 +51,9 @@  static void remote_set_socket(Object *obj, const char *str, Error **errp)
     int fd = atoi(str);
 
     s->ioc = qio_channel_new_fd(fd, &local_err);
+
+    qio_channel_add_watch(s->ioc, G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL,
+                          mpqemu_process_msg, NULL, NULL);
 }
 
 static void remote_instance_init(Object *obj)
diff --git a/include/hw/i386/remote.h b/include/hw/i386/remote.h
index 0f8b861e7a..c3890e57ab 100644
--- a/include/hw/i386/remote.h
+++ b/include/hw/i386/remote.h
@@ -30,4 +30,7 @@  typedef struct RemMachineState {
 #define REMOTE_MACHINE(obj) \
     OBJECT_CHECK(RemMachineState, (obj), TYPE_REMOTE_MACHINE)
 
+gboolean mpqemu_process_msg(QIOChannel *ioc, GIOCondition cond,
+                            gpointer opaque);
+
 #endif