We have the following flaws with it:
1. Layring violation by modifying generic state directly in backends
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.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
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);
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;
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)
+{
+ 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
Hi
On Thu, Dec 4, 2025 at 7:42 PM Vladimir Sementsov-Ogievskiy <
vsementsov@yandex-team.ru> 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 <vsementsov@yandex-team.ru>
---
> 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
>
>
On 05.12.25 18:33, Marc-André Lureau wrote:
> Hi
>
> On Thu, Dec 4, 2025 at 7:42 PM Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru <mailto:vsementsov@yandex-team.ru>> 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.
My actual goal is to "open the doors" for further movement of qemu_char_open() call
to the later point (I came to this idea instead of trying to split .open() into
.open() and .init())..
So I should do something with the logic in chardev_new(), done after qemu_char_open()
call. Alternative is to put it into qemu_char_open(). But I decided, that being here,
it's better to generalize it somehow.
Agree, that setter/getter for filename seems a bit cumbersome here.
Hmm. I think, I have a clearer idea:
add .chr_get_filename() handler and drop the .filename field and qemu_chr_set_filename()
function.
So, qemu_chr_get_filename() will realize the default logic or call .chr_get_filename() if
it exist.
We'll need implementations only for char-socket and char-pty.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru <mailto:vsementsov@yandex-team.ru>>
>
> ---
> 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
>
--
Best regards,
Vladimir
© 2016 - 2025 Red Hat, Inc.