Hello Daniel,
Thank you for your review.
On Thu, Feb 5, 2026 at 9:28 PM Daniel P. Berrangé <[email protected]> wrote:
>
> On Thu, Feb 05, 2026 at 08:58:03PM +0100, Ruslan Ruslichenko wrote:
> > From: Ruslan Ruslichenko <[email protected]>
> >
> > Add initialization of communication channels with
> > remote peer.
> >
> > This includes character device backend, which can be
> > configured based on QOM properties or automatic socket
> > creation based on the machine path. The patch also
> > initializes the signaling event pipes.
> >
> > Signed-off-by: Edgar E. Iglesias <[email protected]>
> > Signed-off-by: Takahiro Nakata <[email protected]>
> > Signed-off-by: Ruslan Ruslichenko <[email protected]>
> > ---
> > hw/core/remote-port.c | 187 ++++++++++++++++++++++++++++++++++
> > include/hw/core/remote-port.h | 16 +++
> > 2 files changed, 203 insertions(+)
> >
> > diff --git a/hw/core/remote-port.c b/hw/core/remote-port.c
> > index c909a825f3..5154c1bc2a 100644
> > --- a/hw/core/remote-port.c
> > +++ b/hw/core/remote-port.c
> > @@ -52,6 +52,58 @@
> > #define REMOTE_PORT_CLASS(klass) \
> > OBJECT_CLASS_CHECK(RemotePortClass, (klass), TYPE_REMOTE_PORT)
> >
> > +static char *rp_sanitize_prefix(RemotePort *s)
> > +{
> > + char *sanitized_name;
> > + char *c;
> > +
> > + sanitized_name = g_strdup(s->prefix);
> > + for (c = sanitized_name; *c != '\0'; c++) {
> > + if (*c == '/') {
> > + *c = '_';
> > + }
> > + }
> > + return sanitized_name;
> > +}
> > +
> > +static char *rp_autocreate_chardesc(RemotePort *s, bool server)
> > +{
> > + char *prefix;
> > + char *chardesc;
> > + int r;
> > +
> > + prefix = rp_sanitize_prefix(s);
> > + r = asprintf(&chardesc, "unix:%s/qemu-rport-%s%s",
> > + machine_path, prefix, server ? ",wait,server" : "");
> > + assert(r > 0);
> > + free(prefix);
> > + return chardesc;
> > +}
> > +
> > +static Chardev *rp_autocreate_chardev(RemotePort *s, char *name)
> > +{
> > + Chardev *chr = NULL;
> > + char *chardesc;
> > + char *s_path;
> > + int r;
> > +
> > + r = asprintf(&s_path, "%s/qemu-rport-%s", machine_path,
> > + rp_sanitize_prefix(s));
> > + assert(r > 0);
> > + if (g_file_test(s_path, G_FILE_TEST_EXISTS)) {
> > + chardesc = rp_autocreate_chardesc(s, false);
> > + chr = qemu_chr_new_noreplay(name, chardesc, false, NULL);
> > + free(chardesc);
> > + }
> > + free(s_path);
> > +
> > + if (!chr) {
> > + chardesc = rp_autocreate_chardesc(s, true);
> > + chr = qemu_chr_new_noreplay(name, chardesc, false, NULL);
> > + free(chardesc);
> > + }
> > + return chr;
> > +}
> >
> > static void rp_reset(DeviceState *dev)
> > {
> > @@ -66,6 +118,127 @@ static void rp_reset(DeviceState *dev)
> >
> > static void rp_realize(DeviceState *dev, Error **errp)
> > {
> > + RemotePort *s = REMOTE_PORT(dev);
> > + int r;
> > + Error *err = NULL;
> > +
> > + s->prefix = object_get_canonical_path(OBJECT(dev));
> > +
> > + if (!qemu_chr_fe_get_driver(&s->chr)) {
> > + char *name;
> > + Chardev *chr = NULL;
> > + static int nr;
> > +
> > + r = asprintf(&name, "rport%d", nr);
> > + nr++;
> > + assert(r > 0);
> > +
> > + if (s->chrdev_id) {
> > + chr = qemu_chr_find(s->chrdev_id);
> > + }
> > +
> > + if (chr) {
> > + /* Found the chardev via commandline */
> > + } else if (s->chardesc) {
> > + chr = qemu_chr_new(name, s->chardesc, NULL);
> > + } else {
> > + if (!machine_path) {
> > + error_report("%s: Missing chardesc prop."
> > + " Forgot -machine-path?",
> > + s->prefix);
> > + exit(EXIT_FAILURE);
> > + }
> > + chr = rp_autocreate_chardev(s, name);
> > + }
>
> Having three different ways to configure the chardev rather
> feels like overkill. IMHO it should be sufficient to just
> accept thue chardev ID as a mandatory argument, and not
> attempt to create chardevs from this code. That would
> in turn seem to avoid the need for the -machine-path arg
> to exist ?
>
I think we can consider using only chrdev_id, for now. This loses some
flexibility, but definitely simplifies things a lot.
>
> > +#ifdef _WIN32
> > + /*
> > + * Create a socket connection between two sockets. We auto-bind
> > + * and read out the port selected by the kernel.
> > + */
> > + {
> > + char *name;
> > + SocketAddress *sock;
> > + int port;
> > + int listen_sk;
> > +
> > + sock = socket_parse("127.0.0.1:0", &error_abort);
> > + listen_sk = socket_listen(sock, 1, &error_abort);
>
> Please avoid introducing new usage of low level sockets APIs. The higher
> level QIOChannelSocket APIs is preferred, that said.....
>
> > +
> > + if (s->event.pipe.read < 0) {
> > + perror("socket read");
> > + exit(EXIT_FAILURE);
> > + }
> > +
> > + {
> > + struct sockaddr_in saddr;
> > + socklen_t slen = sizeof saddr;
> > + int r;
> > +
> > + r = getsockname(listen_sk, (struct sockaddr *) &saddr, &slen);
> > + if (r < 0) {
> > + perror("getsockname");
> > + exit(EXIT_FAILURE);
> > + }
> > + port = htons(saddr.sin_port);
> > + }
> > +
> > + name = g_strdup_printf("127.0.0.1:%d", port);
> > + s->event.pipe.write = inet_connect(name, &error_abort);
> > + g_free(name);
> > + if (s->event.pipe.write < 0) {
> > + perror("socket write");
> > + exit(EXIT_FAILURE);
> > + }
> > +
> > + for (;;) {
> > + struct sockaddr_in saddr;
> > + socklen_t slen = sizeof saddr;
> > + int fd;
> > +
> > + slen = sizeof(saddr);
> > + fd = qemu_accept(listen_sk, (struct sockaddr *)&saddr, &slen);
> > + if (fd < 0 && errno != EINTR) {
> > + close(listen_sk);
> > + return;
> > + } else if (fd >= 0) {
> > + close(listen_sk);
> > + s->event.pipe.read = fd;
> > + break;
> > + }
> > + }
> > +
> > + if (!qemu_set_blocking(s->event.pipe.read, false, &err)) {
> > + error_report("%s: Unable to set non-block for internal pipes",
> > + s->prefix);
> > + exit(EXIT_FAILURE);
> > + }
> > + }
> > +#else
> > + if (!g_unix_open_pipe(s->event.pipes, FD_CLOEXEC, NULL)) {
> > + error_report("%s: Unable to create remort-port internal pipes",
> > + s->prefix);
> > + exit(EXIT_FAILURE);
> > + }
> > +
> > + if (!qemu_set_blocking(s->event.pipe.read, false, &err)) {
> > + error_report("%s: Unable to set non-block for internal pipes",
> > + s->prefix);
> > + exit(EXIT_FAILURE);
> > + }
> > +
> > +#endif
>
> ... I can't help thinking we have this code "self event pipe" design
> somewhere else in QEMU. Ideally we would have a helper API for this
> task and avoid OS conditional code in this device impl.
>
>
Thanks! Looks like EventNotifier may work here and it supports both platforms.
I'll try to use it instead then.
> > }
> >
> > static void rp_unrealize(DeviceState *dev)
> > @@ -73,6 +246,13 @@ static void rp_unrealize(DeviceState *dev)
> > RemotePort *s = REMOTE_PORT(dev);
> >
> > s->finalizing = true;
> > +
> > + info_report("%s: Wait for remote-port to disconnect", s->prefix);
> > + qemu_chr_fe_disconnect(&s->chr);
> > +
> > + close(s->event.pipe.read);
> > + close(s->event.pipe.write);
> > + object_unparent(OBJECT(s->chrdev));
> > }
> >
> > static const VMStateDescription vmstate_rp = {
> > @@ -84,6 +264,12 @@ static const VMStateDescription vmstate_rp = {
> > }
> > };
> >
> > +static Property rp_properties[] = {
> > + DEFINE_PROP_CHR("chardev", RemotePort, chr),
> > + DEFINE_PROP_STRING("chardesc", RemotePort, chardesc),
> > + DEFINE_PROP_STRING("chrdev-id", RemotePort, chrdev_id),
> > +};
>
> This really feels lik
>
> > +
> > static void rp_prop_allow_set_link(const Object *obj, const char *name,
> > Object *val, Error **errp)
> > {
> > @@ -112,6 +298,7 @@ static void rp_class_init(ObjectClass *klass, const
> > void *data)
> > dc->realize = rp_realize;
> > dc->unrealize = rp_unrealize;
> > dc->vmsd = &vmstate_rp;
> > + device_class_set_props_n(dc, rp_properties, ARRAY_SIZE(rp_properties));
> > }
> >
> > static const TypeInfo rp_info = {
> > diff --git a/include/hw/core/remote-port.h b/include/hw/core/remote-port.h
> > index db71071c8e..0f40018cdb 100644
> > --- a/include/hw/core/remote-port.h
> > +++ b/include/hw/core/remote-port.h
> > @@ -56,8 +56,24 @@ typedef struct RemotePortDeviceClass {
> > struct RemotePort {
> > DeviceState parent;
> >
> > + union {
> > + int pipes[2];
> > + struct {
> > + int read;
> > + int write;
> > + } pipe;
> > + } event;
> > + Chardev *chrdev;
> > + CharFrontend chr;
> > bool finalizing;
> >
> > + char *chardesc;
> > + char *chrdev_id;
> > +
> > + const char *prefix;
> > + const char *remote_prefix;
> > +
> > + uint32_t current_id;
> > bool reset_done;
> >
> > #define REMOTE_PORT_MAX_DEVS 1024
> > --
> > 2.43.0
> >
> >
>
> With regards,
> Daniel
> --
> |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o- https://fstop138.berrange.com :|
> |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
>
--
BR,
Ruslan
