[Spice-devel] stat file and API

[Uri Lublin]

On 01/24/2017 07:54 PM, Frediano Ziglio wrote:
> Hi,

Hi Frediano,

>   as it looks somebody is touching this part of code.
>
> Looking at APIs (stat-file.h) we have these functions:
>
> RedStatFile *stat_file_new(unsigned int max_nodes);
> void stat_file_free(RedStatFile *stat_file);
> void stat_file_unlink(RedStatFile *file_stat);
> const char *stat_file_get_shm_name(RedStatFile *stat_file);
> StatNodeRef stat_file_add_node(RedStatFile *stat_file, StatNodeRef parent,
>                                const char *name, int visible);
> uint64_t *stat_file_add_counter(RedStatFile *stat_file, StatNodeRef parent,
>                                 const char *name, int visible);
> void stat_file_remove_node(RedStatFile *stat_file, StatNodeRef ref);
> void stat_file_remove_counter(RedStatFile *stat_file, uint64_t *counter);
>
>
> Currently no code calls the stat_file_remove_* functions (beside the test).

Currently all stats are static and not dynamic (stats to display
are determined at compile time), and they live as long as the
program runs.

>
>
> There is a problem adding/removing nodes/counters. 2 calls with same
> parent+name retry the same node (a counter is implemented as a node) but
> as there is no reference counting on nodes the first remove call will remove
> the node despite the second reference so you'll have an invalid reference
> to a deleted node.

Yes, you are right.
Another option is to fail the second "add".
Why would one add the same statistic twice ?

> This is not a problem as I stated before remove functions are not called
> but what's the point in some API that cannot safely called? They should
> either be removed or fixed.

Theoretically the API can be used to dynamically add/remove stats,
according to user commands (e.g. via qemu monitor).

> As the file is basically an array of nodes (+ header) we could allocate
> an array of reference counters inside RedStatFile and use it (the file
> is used only by a single process for writing).

Alternatively you can add a reference to SpiceStatNode.
Why would you want to add the same counter (node) twice ?

> From the API prospective the code is not much reusable as the filename
> used is basically fixed as
>
>    g_strdup_printf(SPICE_STAT_SHM_NAME, getpid());

Currently it's a simple schema.
All stats of a VM are available via this file.
It is easy to find the file according to the pid of qemu-kvm.
This is important especially if there are several VMs
running on the same host.

Would you like to have multiple stats files of a single VM ?
Of course the current schema can be enhanced to provide it.

>
> this can easily be fixed passing a filename parameter to stat_file_new
> (if we care about code reuse).

That's also possible, but you also have to provide an easy way
for users to pass different stats filename(s) for different VMs.
For example you could enhance qemu-kvm command line -spice option.
You'd have to make sure there are no two VMs that are using the same
file name (for example keep the pid somewhere in the name).

Uri.