| Message ID | 20200602154624.4460-8-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | plugins/next (bug fixes, hwprofile, lockstep) | expand |
On 6/2/20 5:46 PM, Alex Bennée wrote: > This may well end up being anonymous but it should always be unique. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > --- > include/qemu/qemu-plugin.h | 5 +++++ > plugins/api.c | 18 ++++++++++++++++++ > 2 files changed, 23 insertions(+) > > diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h > index bab8b0d4b3a..43c6a9e857f 100644 > --- a/include/qemu/qemu-plugin.h > +++ b/include/qemu/qemu-plugin.h > @@ -335,6 +335,11 @@ struct qemu_plugin_hwaddr *qemu_plugin_get_hwaddr(qemu_plugin_meminfo_t info, > bool qemu_plugin_hwaddr_is_io(const struct qemu_plugin_hwaddr *haddr); > uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr); > > +/* > + * Returns a string representing the device. Plugin must free() it s/free/g_free > + */ > +char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *haddr); > + > typedef void > (*qemu_plugin_vcpu_mem_cb_t)(unsigned int vcpu_index, > qemu_plugin_meminfo_t info, uint64_t vaddr, > diff --git a/plugins/api.c b/plugins/api.c > index bbdc5a4eb46..3c73de8c1c2 100644 > --- a/plugins/api.c > +++ b/plugins/api.c > @@ -303,6 +303,24 @@ uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr > return 0; > } > > +char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *haddr) > +{ > +#ifdef CONFIG_SOFTMMU > + if (haddr && haddr->is_io) { > + MemoryRegionSection *mrs = haddr->v.io.section; > + if (!mrs->mr->name) { > + return g_strdup_printf("anon%08lx", 0xffffffff & (uintptr_t) mrs->mr); > + } else { > + return g_strdup(mrs->mr->name); > + } > + } else { > + return g_strdup("RAM"); > + } > +#else > + return g_strdup("Invalid"); > +#endif > +} > + > /* > * Queries to the number and potential maximum number of vCPUs there > * will be. This helps the plugin dimension per-vcpu arrays. > Reviewed-by: Clement Deschamps <clement.deschamps@greensocs.com>
On Tue, Jun 02, 2020 at 16:46:22 +0100, Alex Bennée wrote: > This may well end up being anonymous but it should always be unique. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > --- > include/qemu/qemu-plugin.h | 5 +++++ > plugins/api.c | 18 ++++++++++++++++++ > 2 files changed, 23 insertions(+) > > diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h > index bab8b0d4b3a..43c6a9e857f 100644 > --- a/include/qemu/qemu-plugin.h > +++ b/include/qemu/qemu-plugin.h > @@ -335,6 +335,11 @@ struct qemu_plugin_hwaddr *qemu_plugin_get_hwaddr(qemu_plugin_meminfo_t info, > bool qemu_plugin_hwaddr_is_io(const struct qemu_plugin_hwaddr *haddr); > uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr); > > +/* > + * Returns a string representing the device. Plugin must free() it > + */ > +char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *haddr); > + > typedef void > (*qemu_plugin_vcpu_mem_cb_t)(unsigned int vcpu_index, > qemu_plugin_meminfo_t info, uint64_t vaddr, > diff --git a/plugins/api.c b/plugins/api.c > index bbdc5a4eb46..3c73de8c1c2 100644 > --- a/plugins/api.c > +++ b/plugins/api.c > @@ -303,6 +303,24 @@ uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr > return 0; > } > > +char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *haddr) > +{ > +#ifdef CONFIG_SOFTMMU > + if (haddr && haddr->is_io) { > + MemoryRegionSection *mrs = haddr->v.io.section; > + if (!mrs->mr->name) { > + return g_strdup_printf("anon%08lx", 0xffffffff & (uintptr_t) mrs->mr); > + } else { > + return g_strdup(mrs->mr->name); > + } > + } else { > + return g_strdup("RAM"); > + } > +#else > + return g_strdup("Invalid"); > +#endif > +} I'd rather use asprintf(3) and strdup(3) here, so that plugins don't have to worry about glib, and on the QEMU side we don't have to worry about plugins calling free() instead of g_free(). Or given that this doesn't look perf-critical, perhaps an easier way out is to wrap the above with: char *g_str = above(); char *ret = strdup(g_str); g_free(g_str); return ret; Not sure we should NULL-check ret, since I don't know whether mrs->mr->name is guaranteed to be non-NULL. Thanks, Emilio
On 6/8/20 5:45 AM, Emilio G. Cota wrote: > On Tue, Jun 02, 2020 at 16:46:22 +0100, Alex Bennée wrote: >> This may well end up being anonymous but it should always be unique. >> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> --- >> include/qemu/qemu-plugin.h | 5 +++++ >> plugins/api.c | 18 ++++++++++++++++++ >> 2 files changed, 23 insertions(+) >> >> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h >> index bab8b0d4b3a..43c6a9e857f 100644 >> --- a/include/qemu/qemu-plugin.h >> +++ b/include/qemu/qemu-plugin.h >> @@ -335,6 +335,11 @@ struct qemu_plugin_hwaddr *qemu_plugin_get_hwaddr(qemu_plugin_meminfo_t info, >> bool qemu_plugin_hwaddr_is_io(const struct qemu_plugin_hwaddr *haddr); >> uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr); >> >> +/* >> + * Returns a string representing the device. Plugin must free() it >> + */ >> +char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *haddr); >> + >> typedef void >> (*qemu_plugin_vcpu_mem_cb_t)(unsigned int vcpu_index, >> qemu_plugin_meminfo_t info, uint64_t vaddr, >> diff --git a/plugins/api.c b/plugins/api.c >> index bbdc5a4eb46..3c73de8c1c2 100644 >> --- a/plugins/api.c >> +++ b/plugins/api.c >> @@ -303,6 +303,24 @@ uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr >> return 0; >> } >> >> +char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *haddr) >> +{ >> +#ifdef CONFIG_SOFTMMU >> + if (haddr && haddr->is_io) { >> + MemoryRegionSection *mrs = haddr->v.io.section; >> + if (!mrs->mr->name) { >> + return g_strdup_printf("anon%08lx", 0xffffffff & (uintptr_t) mrs->mr); >> + } else { >> + return g_strdup(mrs->mr->name); >> + } >> + } else { >> + return g_strdup("RAM"); >> + } >> +#else >> + return g_strdup("Invalid"); >> +#endif >> +} > > I'd rather use asprintf(3) and strdup(3) here, so that plugins don't > have to worry about glib, and on the QEMU side we don't have to worry > about plugins calling free() instead of g_free(). It might make sense, but it should be documented in include/qemu/qemu-plugin.h or docs/devel/tcg-plugins.rst. > > Or given that this doesn't look perf-critical, perhaps an easier way out > is to wrap the above with: > > char *g_str = above(); > char *ret = strdup(g_str); > g_free(g_str); free() ;) > return ret; > > Not sure we should NULL-check ret, since I don't know whether > mrs->mr->name is guaranteed to be non-NULL. > > Thanks, > Emilio >
Emilio G. Cota <cota@braap.org> writes: > On Tue, Jun 02, 2020 at 16:46:22 +0100, Alex Bennée wrote: >> This may well end up being anonymous but it should always be unique. >> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> --- >> include/qemu/qemu-plugin.h | 5 +++++ >> plugins/api.c | 18 ++++++++++++++++++ >> 2 files changed, 23 insertions(+) >> >> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h >> index bab8b0d4b3a..43c6a9e857f 100644 >> --- a/include/qemu/qemu-plugin.h >> +++ b/include/qemu/qemu-plugin.h >> @@ -335,6 +335,11 @@ struct qemu_plugin_hwaddr *qemu_plugin_get_hwaddr(qemu_plugin_meminfo_t info, >> bool qemu_plugin_hwaddr_is_io(const struct qemu_plugin_hwaddr *haddr); >> uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr); >> >> +/* >> + * Returns a string representing the device. Plugin must free() it >> + */ >> +char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *haddr); >> + >> typedef void >> (*qemu_plugin_vcpu_mem_cb_t)(unsigned int vcpu_index, >> qemu_plugin_meminfo_t info, uint64_t vaddr, >> diff --git a/plugins/api.c b/plugins/api.c >> index bbdc5a4eb46..3c73de8c1c2 100644 >> --- a/plugins/api.c >> +++ b/plugins/api.c >> @@ -303,6 +303,24 @@ uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr >> return 0; >> } >> >> +char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *haddr) >> +{ >> +#ifdef CONFIG_SOFTMMU >> + if (haddr && haddr->is_io) { >> + MemoryRegionSection *mrs = haddr->v.io.section; >> + if (!mrs->mr->name) { >> + return g_strdup_printf("anon%08lx", 0xffffffff & (uintptr_t) mrs->mr); >> + } else { >> + return g_strdup(mrs->mr->name); >> + } >> + } else { >> + return g_strdup("RAM"); >> + } >> +#else >> + return g_strdup("Invalid"); >> +#endif >> +} > > I'd rather use asprintf(3) and strdup(3) here, so that plugins don't > have to worry about glib, and on the QEMU side we don't have to worry > about plugins calling free() instead of g_free(). AFAIK you can actually mix free/g_free because g_free is just a NULL checking wrapper around free. However ideally I'd be passing a non-freeable const char to the plugin but I didn't want to expose pointers deep inside of QEMU's guts although maybe I'm just being paranoid there given you can easily gdb the combined operation anyway. Perhaps there is a need for a separate memory region where we can store copies of strings we have made for the plugins? > Or given that this doesn't look perf-critical, perhaps an easier way out > is to wrap the above with: > > char *g_str = above(); > char *ret = strdup(g_str); > g_free(g_str); > return ret; > > Not sure we should NULL-check ret, since I don't know whether > mrs->mr->name is guaranteed to be non-NULL. Experimentation showed you can get null mrs->name has a result of a region being subdivided due to an some operations. That said in another thread Peter was uncomfortable about exposing this piece of information to plugins. Maybe we should only expose something based on the optional -device foo,id=bar parameter? > > Thanks, > Emilio -- Alex Bennée
On Mon, Jun 08, 2020 at 09:06:17 +0100, Alex Bennée wrote: > Emilio G. Cota <cota@braap.org> writes: > > I'd rather use asprintf(3) and strdup(3) here, so that plugins don't > > have to worry about glib, and on the QEMU side we don't have to worry > > about plugins calling free() instead of g_free(). > > AFAIK you can actually mix free/g_free because g_free is just a NULL > checking wrapper around free. I was just going with the documentation, but you're right: https://github.com/GNOME/glib/blob/mainline/glib/gmem.c#L196 > void > g_free (gpointer mem) > { > free (mem); > TRACE(GLIB_MEM_FREE((void*) mem)); > } The NULL-pointer check is done by free(3), though. > However ideally I'd be passing a > non-freeable const char to the plugin but I didn't want to expose > pointers deep inside of QEMU's guts although maybe I'm just being > paranoid there given you can easily gdb the combined operation anyway. > > Perhaps there is a need for a separate memory region where we can store > copies of strings we have made for the plugins? I agree with the idea of not exposing internal pointers to plugins (e.g. we don't pass a CPUState *, only an opaque handle) so I'm OK with returning a dup'ed string here. (snip) > That said in another > thread Peter was uncomfortable about exposing this piece of information > to plugins. Maybe we should only expose something based on the optional > -device foo,id=bar parameter? I have no opinion on whether exposing this is a good idea. If it turns out that it is, please have my Reviewed-by: Emilio G. Cota <cota@braap.org> Thanks, Emilio
Emilio G. Cota <cota@braap.org> writes: > On Mon, Jun 08, 2020 at 09:06:17 +0100, Alex Bennée wrote: >> Emilio G. Cota <cota@braap.org> writes: >> > I'd rather use asprintf(3) and strdup(3) here, so that plugins don't >> > have to worry about glib, and on the QEMU side we don't have to worry >> > about plugins calling free() instead of g_free(). >> >> AFAIK you can actually mix free/g_free because g_free is just a NULL >> checking wrapper around free. > > I was just going with the documentation, but you're right: > > https://github.com/GNOME/glib/blob/mainline/glib/gmem.c#L196 >> void >> g_free (gpointer mem) >> { >> free (mem); >> TRACE(GLIB_MEM_FREE((void*) mem)); >> } > > The NULL-pointer check is done by free(3), though. > >> However ideally I'd be passing a >> non-freeable const char to the plugin but I didn't want to expose >> pointers deep inside of QEMU's guts although maybe I'm just being >> paranoid there given you can easily gdb the combined operation anyway. >> >> Perhaps there is a need for a separate memory region where we can store >> copies of strings we have made for the plugins? > > I agree with the idea of not exposing internal pointers to plugins > (e.g. we don't pass a CPUState *, only an opaque handle) so I'm OK > with returning a dup'ed string here. How about a g_intern_string() as a non-freeable const char that can also be treated as canonical? > > (snip) >> That said in another >> thread Peter was uncomfortable about exposing this piece of information >> to plugins. Maybe we should only expose something based on the optional >> -device foo,id=bar parameter? > > I have no opinion on whether exposing this is a good idea. If it turns > out that it is, please have my > > Reviewed-by: Emilio G. Cota <cota@braap.org> > > Thanks, > > Emilio -- Alex Bennée
On Tue, Jun 09, 2020 at 12:09:54 +0100, Alex Bennée wrote: > How about a g_intern_string() as a non-freeable const char that can also > be treated as canonical? I like it. Didn't know about g_intern_string (I see it's implemented as an append-only hash table protected by a lock). Cheers, Emilio
diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h index bab8b0d4b3a..43c6a9e857f 100644 --- a/include/qemu/qemu-plugin.h +++ b/include/qemu/qemu-plugin.h @@ -335,6 +335,11 @@ struct qemu_plugin_hwaddr *qemu_plugin_get_hwaddr(qemu_plugin_meminfo_t info, bool qemu_plugin_hwaddr_is_io(const struct qemu_plugin_hwaddr *haddr); uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr); +/* + * Returns a string representing the device. Plugin must free() it + */ +char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *haddr); + typedef void (*qemu_plugin_vcpu_mem_cb_t)(unsigned int vcpu_index, qemu_plugin_meminfo_t info, uint64_t vaddr, diff --git a/plugins/api.c b/plugins/api.c index bbdc5a4eb46..3c73de8c1c2 100644 --- a/plugins/api.c +++ b/plugins/api.c @@ -303,6 +303,24 @@ uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr return 0; } +char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *haddr) +{ +#ifdef CONFIG_SOFTMMU + if (haddr && haddr->is_io) { + MemoryRegionSection *mrs = haddr->v.io.section; + if (!mrs->mr->name) { + return g_strdup_printf("anon%08lx", 0xffffffff & (uintptr_t) mrs->mr); + } else { + return g_strdup(mrs->mr->name); + } + } else { + return g_strdup("RAM"); + } +#else + return g_strdup("Invalid"); +#endif +} + /* * Queries to the number and potential maximum number of vCPUs there * will be. This helps the plugin dimension per-vcpu arrays.
This may well end up being anonymous but it should always be unique. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- include/qemu/qemu-plugin.h | 5 +++++ plugins/api.c | 18 ++++++++++++++++++ 2 files changed, 23 insertions(+) -- 2.20.1