Re: [PATCH v1 7/9] plugins: add API to return a name for a IO device

[Alex Bennée]

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