Re: [LTP] [RFC][PATCH 2/4][RT] Integration of xml into stats.

[Darren H. <dv...@us...>]

Gilles Carry wrote:
> This patch adds a new function into libstat:
> xml_stats_container_save which handles an xml_stream_t instead of a file.
> It dumps the same data stats_container_save with XML format.
> 
> Also added two fields to stats: timestamp and cpuid.
> 
> Compilation is conditional to LIB_XML.
> ---
>  testcases/realtime/include/libstats.h |   16 +++++++++++
>  testcases/realtime/lib/libstats.c     |   47 +++++++++++++++++++++++++++++++++
>  2 files changed, 63 insertions(+), 0 deletions(-)
> 
> diff --git a/testcases/realtime/include/libstats.h b/testcases/realtime/include/libstats.h
> index 05c26d8..591d94f 100644
> --- a/testcases/realtime/include/libstats.h
> +++ b/testcases/realtime/include/libstats.h
> @@ -45,6 +45,10 @@
>  #include <errno.h>
>  #include <unistd.h>
>  #include <math.h>
> +#include <librttest.h>

Hrm, seems odd to require this include to dump to XML.  Why do we need it?

> +#ifdef LIB_XML
> +#include <libxml.h>
> +#endif
> 
>  #define MIN(A,B) ((A)<(B)?(A):(B))
>  #define MAX(A,B) ((A)>(B)?(A):(B))
> @@ -52,6 +56,8 @@
>  typedef struct stats_record {
>  	long x;
>  	long y;
> +	nsec_t t; /* timestamp */
> +	int c; /* cpu id */

hrm... thinking...

I like getting the timestamp regardless of if xml is used.  Will watch 
for arch issues below re the cpu id.  Also, x and y were short names 
because it was sufficient to identify their purpose, let's use more 
descriptive names for t and c, like timestamp and cpu, then you won't 
need the comments and the code where it's used will be self evident.

>  } stats_record_t;
> 
>  typedef struct stats_container {
> @@ -162,4 +168,14 @@ void stats_hist_print(stats_container_t *hist);
>   */
>  int stats_container_save(char *filename, char *title, char *labelx, char *labely, stats_container_t *data, char *mode);
> 
> +#ifdef LIB_XML
> +/* stats_container_xml_save - save the x,y data to an xml file.
> + * xs: the stream previously created with xml_stream_init.
> + * labelx: the x-axis label
> + * labely: the y-axis label
> + * mode: "points" "lines" "steps" etc, see gnuplot help for plotting types
> + */
> +int xml_stats_container_save(xml_stream_t *xs, char *title, char *xlabel, char *ylabel, stats_container_t *data, char *mode);
> +#endif
> +
>  #endif /* LIBSTAT_H */
> diff --git a/testcases/realtime/lib/libstats.c b/testcases/realtime/lib/libstats.c
> index 29e7ea7..ed9b9a2 100644
> --- a/testcases/realtime/lib/libstats.c
> +++ b/testcases/realtime/lib/libstats.c
> @@ -43,6 +43,9 @@
>  #include <unistd.h>
>  #include <math.h>
>  #include <libstats.h>
> +#ifdef LIB_XML
> +#include <libxml.h>
> +#endif
> 
>  int save_stats = 0;
> 
> @@ -355,3 +358,47 @@ int stats_container_save(char *filename, char *title, char *xlabel, char *ylabel
> 
>      return 0;
>  }
> +
> +#ifdef LIB_XML
> +int xml_stats_container_save(xml_stream_t *xs, char *title, char *xlabel, char *ylabel, stats_container_t *data, char *mode)
> +{
> +	int i;
> +	long minx = 0, maxx = 0, miny = 0, maxy = 0;
> +	stats_record_t *rec;
> +
> +	xml_start_tag(xs, "stats");
> +	minx = maxx = data->records[0].x;
> +	miny = maxy = data->records[0].y;
> +	xml_entry(xs, "title",title);
> +	xml_entry(xs, "xlabel",xlabel);
> +	xml_entry(xs, "ylabel",ylabel);
> +	xml_entry(xs, "mode",mode);
> +	xml_start_tag(xs, "data");
> +
> +	for (i = 0; i < data->size; i++) {
> +		rec = &data->records[i];
> +		minx = MIN(minx, rec->x);
> +		maxx = MAX(maxx, rec->x);
> +		miny = MIN(miny, rec->y);
> +		maxy = MAX(maxy, rec->y);
> +
> +		xml_start_tag(xs, "e");

"e" ?  Seems to me one of the purposes of using xml is to generate human 
readable output.  "e" doesn't say much about what the tag contains.

> +		xml_entry(xs, "r", "%d", i);
> +		xml_entry(xs, "x", "%ld", rec->x);
> +		xml_entry(xs, "y", "%ld", rec->y);
> +		xml_entry(xs, "t", "%lld", rec->t);
> +		xml_entry(xs, "c", "%d", rec->c);

 From a purely xml point of view, all of these things look a lot more 
like attributes than CDATA.  What do you think about using xml attrs 
rather than CDATA?  perhaps using something like:

Also, I think xml_entry() and xml_empty_tag() are confusing. 
xml_entry() is actually a tag every bit as much as the others, so why 
not stick with the same term?

We might be able to use something with vargs to add attributes to an 
xml_tag() call.... Thoughts?

> +		xml_end_tag(xs, "e");
> +	}
> +
> +	xml_end_tag(xs, "data");
> +	xml_entry(xs, "sample-size", "%ld", data->size);
> +	xml_entry(xs, "minx", "%ld", minx);
> +	xml_entry(xs, "maxx", "%ld", maxx);
> +	xml_entry(xs, "miny", "%ld", miny);
> +	xml_entry(xs, "maxy", "%ld", maxy);
> +	xml_end_tag(xs, "stats");
> +
> +	return 0;
> +}
> +#endif


-- 
Darren Hart
IBM Linux Technology Center
Real-Time Linux Team