Hi On Thu, Dec 4, 2025 at 7:42 PM Vladimir Sementsov-Ogievskiy < [email protected]> wrote:
> We have the following flaws with it: > > 1. Layring violation by modifying generic state directly in backends > > Well, it's a parent field that can be modified, I am not sure it qualifies as such > 2. Tricky generic logic: we should check, did backend set the > generic state field, and fill it when not. > > Let's fix them all by making filename a private field with getter > and setter. And move the "default logic" into getter. > > The tradeoff is that your implementation will do more allocation/free, but I don't think we care much here. I am not sure we gain much overall. > Signed-off-by: Vladimir Sementsov-Ogievskiy <[email protected]> --- > chardev/char-pty.c | 4 +++- > chardev/char-socket.c | 17 ++++++++--------- > chardev/char.c | 8 ++------ > hw/misc/ivshmem-pci.c | 4 ++-- > include/chardev/char.h | 21 ++++++++++++++++++++- > 5 files changed, 35 insertions(+), 19 deletions(-) > > diff --git a/chardev/char-pty.c b/chardev/char-pty.c > index 047aade09e..f4294679be 100644 > --- a/chardev/char-pty.c > +++ b/chardev/char-pty.c > @@ -336,6 +336,7 @@ static bool pty_chr_open(Chardev *chr, ChardevBackend > *backend, Error **errp) > int master_fd, slave_fd; > char *name; > char *path = backend->u.pty.data->path; > + g_autofree char *filename = NULL; > > s = PTY_CHARDEV(chr); > > @@ -351,7 +352,8 @@ static bool pty_chr_open(Chardev *chr, ChardevBackend > *backend, Error **errp) > return false; > } > > - chr->filename = g_strdup_printf("pty:%s", s->pty_name); > + filename = g_strdup_printf("pty:%s", s->pty_name); > + qemu_chr_set_filename(chr, filename); > qemu_printf("char device redirected to %s (label %s)\n", > s->pty_name, chr->label); > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c > index 31c9acd164..9387760009 100644 > --- a/chardev/char-socket.c > +++ b/chardev/char-socket.c > @@ -384,8 +384,7 @@ static void tcp_chr_free_connection(Chardev *chr) > s->sioc = NULL; > object_unref(OBJECT(s->ioc)); > s->ioc = NULL; > - g_free(chr->filename); > - chr->filename = NULL; > + qemu_chr_set_filename(chr, NULL); > tcp_chr_change_state(s, TCP_CHARDEV_STATE_DISCONNECTED); > } > > @@ -443,11 +442,11 @@ static void > update_disconnected_filename(SocketChardev *s) > { > Chardev *chr = CHARDEV(s); > > - g_free(chr->filename); > if (s->addr) { > - chr->filename = qemu_chr_socket_address(s, "disconnected:"); > + g_autofree char *filename = qemu_chr_socket_address(s, > "disconnected:"); > + qemu_chr_set_filename(chr, filename); > } else { > - chr->filename = g_strdup("disconnected:socket"); > + qemu_chr_set_filename(chr, "disconnected:socket"); > } > } > > @@ -638,9 +637,9 @@ static void tcp_chr_connect(void *opaque) > { > Chardev *chr = CHARDEV(opaque); > SocketChardev *s = SOCKET_CHARDEV(opaque); > + g_autofree char *filename = qemu_chr_compute_filename(s); > > - g_free(chr->filename); > - chr->filename = qemu_chr_compute_filename(s); > + qemu_chr_set_filename(chr, filename); > > tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTED); > update_ioc_handlers(s); > @@ -1000,8 +999,8 @@ static void tcp_chr_accept_server_sync(Chardev *chr) > { > SocketChardev *s = SOCKET_CHARDEV(chr); > QIOChannelSocket *sioc; > - info_report("QEMU waiting for connection on: %s", > - chr->filename); > + g_autofree char *filename = qemu_chr_get_filename(chr); > + info_report("QEMU waiting for connection on: %s", filename); > tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING); > sioc = qio_net_listener_wait_client(s->listener); > tcp_chr_set_client_ioc_name(chr, sioc); > diff --git a/chardev/char.c b/chardev/char.c > index 0dc792b88f..bdd907f015 100644 > --- a/chardev/char.c > +++ b/chardev/char.c > @@ -309,7 +309,7 @@ static void char_finalize(Object *obj) > if (chr->fe) { > chr->fe->chr = NULL; > } > - g_free(chr->filename); > + qemu_chr_set_filename(chr, NULL); > g_free(chr->label); > if (chr->logfd != -1) { > close(chr->logfd); > @@ -796,7 +796,7 @@ static int qmp_query_chardev_foreach(Object *obj, void > *data) > ChardevInfo *value = g_malloc0(sizeof(*value)); > > value->label = g_strdup(chr->label); > - value->filename = g_strdup(chr->filename); > + value->filename = qemu_chr_get_filename(chr); > value->frontend_open = chr->fe && chr->fe->fe_is_open; > > QAPI_LIST_PREPEND(*list, value); > @@ -1025,10 +1025,6 @@ static Chardev *chardev_new(const char *id, const > char *typename, > return NULL; > } > > - if (!chr->filename) { > - chr->filename = g_strdup(typename + 8); > - } > - > return chr; > } > > diff --git a/hw/misc/ivshmem-pci.c b/hw/misc/ivshmem-pci.c > index 636d0b83de..2c7b987241 100644 > --- a/hw/misc/ivshmem-pci.c > +++ b/hw/misc/ivshmem-pci.c > @@ -873,10 +873,10 @@ static void ivshmem_common_realize(PCIDevice *dev, > Error **errp) > host_memory_backend_set_mapped(s->hostmem, true); > } else { > Chardev *chr = qemu_chr_fe_get_driver(&s->server_chr); > + char *filename = qemu_chr_get_filename(chr); > Should be auto-free, since returned allocated > assert(chr); > > - IVSHMEM_DPRINTF("using shared memory server (socket = %s)\n", > - chr->filename); > + IVSHMEM_DPRINTF("using shared memory server (socket = %s)\n", > filename); > > /* we allocate enough space for 16 peers and grow as needed */ > resize_peers(s, 16); > diff --git a/include/chardev/char.h b/include/chardev/char.h > index d36e50b99e..ffeb4a4e3b 100644 > --- a/include/chardev/char.h > +++ b/include/chardev/char.h > @@ -62,7 +62,7 @@ struct Chardev { > QemuMutex chr_write_lock; > CharFrontend *fe; > char *label; > - char *filename; > + char *_filename; > Why rename the field? we don't have a convention to have "private" fields with _ prefix afaik. > int logfd; > int be_open; > /* used to coordinate the chardev-change special-case: */ > @@ -72,6 +72,25 @@ struct Chardev { > DECLARE_BITMAP(features, QEMU_CHAR_FEATURE_LAST); > }; > > +static inline char *qemu_chr_get_filename(Chardev *chr) > Let's avoid code in headers. > +{ > + const char *typename; > + > + if (chr->_filename) { > + return g_strdup(chr->_filename); > + } > + > + typename = object_get_typename(OBJECT(chr)); > + assert(g_str_has_prefix(typename, "chardev-")); > + return g_strdup(typename + 8); > +} > + > +static inline void qemu_chr_set_filename(Chardev *chr, const char > *filename) > +{ > + g_free(chr->_filename); > + chr->_filename = g_strdup(filename); > +} > + > /** > * qemu_chr_new_from_opts: > * @opts: see qemu-config.c for a list of valid options > -- > 2.48.1 > >
