[lttng-dev] Prototype MI
Jonathan Rajotte
jonathan.r.julien at gmail.com
Thu Mar 13 16:51:46 EDT 2014
On Thu, Mar 13, 2014 at 12:00:50PM -0400, David Goulet wrote:
> Hey!
>
> (putting lttng-dev in CC, this is a very relevant discussion that should
> be public).
>
> Something I'm wondering here before we go further. Is this code wouldn't
> be better off in libconfig (src/common/config)?
>
> A reason would be to have it available in a library maybe at some point
> in time like for instance "lttng_list_sessions(int output_format, ...)"
> and output_format is set to XML for example.
As discussed on the IRC, adding a global or command specific option for
MI that promote different output format does not add any complexity. So
the final option will be : --mi <output_format>. XML will be the
default. So "lttng_list_sessions(int output_format, ...)" make sense.
>
> One of the problem right now due to a very limited public API we have to
> cope with is the fact that right now, on the lttng client side, we can't
> list everything so this mi interface will only be used for a subset of
> what we can offer to the user (which the save feature addresses being on
> the session daemon side).
>
> I'm not saying that we should reimplement the whole API with new
> functions that sends back the mi output but at least in term of code
> location in the repository, it could be a good idea to think that not
> only the lttng command line might use it *especially* since libconfig is
> almost doing the same thing for the save/load feature.
Make sense to move it there, sure the 'libconfig' name isn't
representative of MI but for now the MI would be highly dependant on the
libxml wrapper present in config.h. After all the point of an MI is to
standardize communications between application and putting it in a common
lib does promote standardization.
>
> Food for thoughts ;)
>
> Oh one thing, last path, all switch cases are tabbed for no reason,
> maybe your editor did that automagically ? ;)
Magic :P (let say we have to re-read the coding Style...)
>
> Cheers!
> David
>
And now the code review :)
> On 13 Mar (11:45:33), Jérémie Galarneau wrote:
> > Hi,
> >
> > Inlining the first patch, see comments throughout. I'll review the
> > other patches in separate e-mails.
> >
> > Is there a reason why you define an mi_writer wrapper over
> > config_writer? Do you expect you'll redefine "mi_writer" as a
> > full-fledged structure (instead of a typedef)?
At first we did this only to separate us from the 'config' nomenclature.
We did not expect to redefine it but with the appearance of
error handling and temporary file management we currently have redefined
it to a full-fledged structure.
> >
> > Also, mi-internal.h and mi.h can be merged if you don't plan on making
> > them public. We typically only define "*-internal.h" headers when
> > there is already an equivalent public header already.
Understood
> >
> > Finally, please run the checkpatch.pl script in lttng-tools/extras/ on
> > your patches. It will point out most style issues.
> >
Will do as for all the style annotations.
> > Regards,
> > Jérémie
> >
> > > From 205eeaf19d0246b6db0517d9c22727b82f5e444e Mon Sep 17 00:00:00 2001
> > > From: Jonathan Rajotte <jonathan.r.julien at gmail.com>
> > > Date: Fri, 7 Mar 2014 09:22:13 -0500
> > > Subject: [PATCH 1/3] MI: basic mi utility function
> > >
> > >
> > > Signed-off-by: Jonathan Rajotte <jonathan.r.julien at gmail.com>
> > > ---
> > > src/bin/lttng/Makefile.am | 1 +
> > > src/bin/lttng/mi-internal.h | 69 ++++++++++++++++++++
> > > src/bin/lttng/mi.c | 130 +++++++++++++++++++++++++++++++++++++
> > > src/bin/lttng/mi.h | 150 +++++++++++++++++++++++++++++++++++++++++++
> > > 4 files changed, 350 insertions(+)
> > > create mode 100644 src/bin/lttng/mi-internal.h
> > > create mode 100644 src/bin/lttng/mi.c
> > > create mode 100644 src/bin/lttng/mi.h
> > >
> > > diff --git a/src/bin/lttng/Makefile.am b/src/bin/lttng/Makefile.am
> > > index 5a1eb6a..93c39fa 100644
> > > --- a/src/bin/lttng/Makefile.am
> > > +++ b/src/bin/lttng/Makefile.am
> > > @@ -16,6 +16,7 @@ lttng_SOURCES = command.h conf.c conf.h commands/start.c \
> > > commands/snapshot.c \
> > > commands/save.c \
> > > commands/load.c \
> > > + mi.c \
> > > utils.c utils.h lttng.c
> > >
> > > lttng_LDADD = $(top_builddir)/src/lib/lttng-ctl/liblttng-ctl.la \
> > > diff --git a/src/bin/lttng/mi-internal.h b/src/bin/lttng/mi-internal.h
> > > new file mode 100644
> > > index 0000000..346f715
> > > --- /dev/null
> > > +++ b/src/bin/lttng/mi-internal.h
> > > @@ -0,0 +1,69 @@
> > > +/*
> > > + * Copyright (C) 2014 - Jonathan Rajotte <jonathan.r.julien at gmail.com>
> > > + * - Olivier Cotte <olivier.cotte at polymtl.ca>
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify it
> > > + * under the terms of the GNU General Public License, version 2 only, as
> > > + * published by the Free Software Foundation.
> > > + *
> > > + * This program is distributed in the hope that it will be useful, but WITHOUT
> > > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> > > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> > > + * more details.
> > > + *
> > > + * You should have received a copy of the GNU General Public License along with
> > > + * this program; if not, write to the Free Software Foundation, Inc., 51
> > > + * Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> > > + */
> > > +
> > > +#ifndef MI_INTERNAL_H
> > > +#define MI_INTERNAL_H
> > > +
> > > +// Strings related to command
> >
> > Don't use C99 comments, prefer the /* ... */ form
> >
> > > +const char * const mi_element_command;
> > > +const char * const mi_element_command_version;
> > > +const char * const mi_element_command_list;
> > > +
> > > +// Strings related to command: version
> >
> > Don't use C99 comments, prefer the /* ... */ form
> >
> > > +const char * const mi_element_version_str;
> > > +const char * const mi_element_version_web;
> > > +const char * const mi_element_version_name;
> > > +const char * const mi_element_version_major;
> > > +const char * const mi_element_version_minor;
> > > +const char * const mi_element_version_license;
> > > +const char * const mi_element_version_patch_level;
> > > +const char * const mi_element_version_description;
> > > +
> > > +// Strings related to command: list -k (kernel)
> >
> > Don't use C99 comments, prefer the /* ... */ form
> >
> > > +const char * const mi_element_list_kernel_event_tracepoint;
> > > +const char * const mi_element_list_kernel_event_function;
> > > +const char * const mi_element_list_kernel_event_probe;
> > > +const char * const mi_element_list_kernel_event_function_entry;
> > > +const char * const mi_element_list_kernel_event_syscall;
> > > +const char * const mi_element_list_kernel_event_noop;
> > > +const char * const mi_element_list_kernel_event_all;
> > > +const char * const mi_element_list_events_flag;
> > > +const char * const mi_element_list_event_flag;
> > > +const char * const mi_element_list_event_type;
> > > +const char * const mi_element_list_event_name;
> > > +const char * const mi_element_list_loglevel_type;
> > > +const char * const mi_element_list_loglevel;
> > > +const char * const mi_element_list_event_enabled;
> > > +const char * const mi_element_list_event_exclusion;
> > > +const char * const mi_element_list_event_filter;
> > > +const char * const mi_element_list_loglevel_str;
> > > +const char * const mi_element_list_event_enabled_str;
> > > +const char * const mi_element_list_event_exclusion_str;
> > > +const char * const mi_element_list_event_filter_str;
> > > +const char * const mi_element_list_event_addr;
> > > +const char * const mi_element_list_event_offset;
> > > +const char * const mi_element_list_event_symbol;
> > > +
> > > +
> >
> > Remove the extra whitespace.
> >
> > > +// Strings related to command: list -u (userspace)
> >
> > Don't use C99 comments, prefer the /* ... */ form
> >
> > > +const char * const mi_element_list_ust_jul_events;
> > > +const char * const mi_element_list_ust_jul_event;
> > > +const char * const mi_element_list_ust_curr_pid;
> > > +const char * const mi_element_list_ust_cmdline;
> > > +
> > > +#endif /* MI_INTERNAL_H */
> > > diff --git a/src/bin/lttng/mi.c b/src/bin/lttng/mi.c
> > > new file mode 100644
> > > index 0000000..016b52a
> > > --- /dev/null
> > > +++ b/src/bin/lttng/mi.c
> > > @@ -0,0 +1,130 @@
> > > +/*
> > > + * Copyright (C) 2014 - Jonathan Rajotte <jonathan.r.julien at gmail.com>
> > > + * - Olivier Cotte <olivier.cotte at polymtl.ca>
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify it
> > > + * under the terms of the GNU General Public License, version 2 only, as
> > > + * published by the Free Software Foundation.
> > > + *
> > > + * This program is distributed in the hope that it will be useful, but WITHOUT
> > > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> > > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> > > + * more details.
> > > + *
> > > + * You should have received a copy of the GNU General Public License along with
> > > + * this program; if not, write to the Free Software Foundation, Inc., 51
> > > + * Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> > > + */
> > > +
> > > +#include <common/config/config.h>
> > > +#include "mi-internal.h"
> > > +#include "mi.h"
> > > +
> >
> > Some of these strings are already defined in config.c. Is there a
> > reason why you redefine them here?
> > We could point to them at least.
We decided not to point to these because we considered the MI and
config(load-save) two different things, one internal(load/save) to sessiond
and the other one a public "interface". You could want more specifics
names for internals setting and more generics ones for MI.
> >
> > David, any thoughts on this?
> >
> > > +/* Strings related to command */
> > > +const char * const mi_element_command = "command";
> > > +const char * const mi_element_command_version = "version";
> > > +const char * const mi_element_command_list = "list";
> > > +
> > > +/* Strings related to command: version */
> > > +const char * const mi_element_version_str = "string";
> > > +const char * const mi_element_version_web = "url";
> > > +const char * const mi_element_version_name = "name";
> > > +const char * const mi_element_version_major = "major";
> > > +const char * const mi_element_version_minor = "minor";
> > > +const char * const mi_element_version_license = "license";
> > > +const char * const mi_element_version_patch_level = "patchLevel";
> > > +const char * const mi_element_version_description = "description";
> > > +
> > > +/* Strings related to command: list */
> > > +const char * const mi_element_list_kernel_event_tracepoint = "TRACEPOINT";
> > > +const char * const mi_element_list_kernel_event_function = "FUNCTION";
> > > +const char * const mi_element_list_kernel_event_probe = "PROBE";
> > > +const char * const mi_element_list_kernel_event_function_entry = "FUNCTION_ENTRY";
> > > +const char * const mi_element_list_kernel_event_syscall = "SYSCALL";
> > > +const char * const mi_element_list_kernel_event_noop = "NOOP";
> > > +const char * const mi_element_list_kernel_event_all = "ALL";
> > > +const char * const mi_element_list_events_flag = "events";
> > > +const char * const mi_element_list_event_flag = "event";
> > > +const char * const mi_element_list_event_type = "type";
> > > +const char * const mi_element_list_event_name = "name";
> > > +const char * const mi_element_list_loglevel_type = "loglevel_type";
> > > +const char * const mi_element_list_loglevel = "loglevel";
> > > +const char * const mi_element_list_event_enabled = "enabled";
> > > +const char * const mi_element_list_event_exclusion = "exclusion";
> > > +const char * const mi_element_list_event_filter = "filter";
> > > +const char * const mi_element_list_loglevel_str = "loglevel_string";
> > > +const char * const mi_element_list_event_enabled_str = "enabled_string";
> > > +const char * const mi_element_list_event_exclusion_str = "exclusion_string";
> > > +const char * const mi_element_list_event_filter_str = "filter_string";
> > > +const char * const mi_element_list_event_addr = "addr";
> > > +const char * const mi_element_list_event_offset = "offset";
> > > +const char * const mi_element_list_event_symbol = "symbol";
> > > +
> > > +const char * const mi_element_list_ust_jul_events = "jul_events";
> > > +const char * const mi_element_list_ust_jul_event = "jul_event";
> > > +const char * const mi_element_list_ust_curr_pid = "curr_pid";
> > > +const char * const mi_element_list_ust_cmdline = "cmdline";
> > > +
> >
> > Please add the LTTNG_HIDDEN annotation to all functions in this file.
> >
> > > +mi_writer *mi_writer_create(int fd_output)
> > > +{
> > > + //TODO JRJ: handle err ... with tmp file
> > > + return config_writer_create(fd_output);
> >
> > Use tabs instead of spaces. Applies throughout
> >
> > > +}
> > > +
> > > +int mi_writer_destroy(mi_writer *writer)
> > > +{
> > > + return config_writer_destroy(writer);
> > > +}
> > > +
> > > +int mi_writer_command_open(mi_writer *writer, const char* command)
> >
> > Prefer
> >
> > const char *command
> >
> > to
> >
> > const char* command.
> >
> > > +{
> > > + int ret;
> > > + ret = mi_writer_open_element(writer, mi_element_command);
> > > + if (ret) {
> > > + goto end;
> > > + }
> > > + ret = mi_writer_open_element(writer, command);
> > > +end:
> > > + return ret;
> > > +}
> > > +
> > > +int mi_writer_command_close(mi_writer *writer)
> > > +{
> > > + return mi_writer_close_element(writer);
> > > +}
> > > +
> > > +int mi_writer_open_element(mi_writer *writer,
> > > + const char *element_name)
> > > +{
> > > + return config_writer_open_element(writer, element_name);
> > > +}
> > > +
> > > +int mi_writer_close_element(mi_writer *writer)
> > > +{
> > > + return config_writer_close_element(writer);
> > > +}
> > > +
> > > +int mi_writer_write_element_unsigned_int(mi_writer *writer,
> > > + const char *element_name, uint64_t value)
> > > +{
> > > + return config_writer_write_element_unsigned_int(writer, element_name, value);
> >
> > Line over 80 characters.
> >
> > > +}
> > > +
> > > +int mi_writer_write_element_signed_int(mi_writer *writer,
> > > + const char *element_name, int64_t value)
> > > +{
> > > + return config_writer_write_element_signed_int(writer, element_name, value);
> > > +}
> > > +
> > > +int mi_writer_write_element_bool(mi_writer *writer,
> > > + const char *element_name, int value)
> > > +{
> > > + return config_writer_write_element_bool(writer, element_name, value);
> > > +}
> > > +
> > > +int mi_writer_write_element_string(mi_writer *writer,
> > > + const char *element_name, const char *value)
> > > +{
> > > + return config_writer_write_element_string(writer, element_name, value);
> > > +}
> > > +
> > > diff --git a/src/bin/lttng/mi.h b/src/bin/lttng/mi.h
> > > new file mode 100644
> > > index 0000000..d509273
> > > --- /dev/null
> > > +++ b/src/bin/lttng/mi.h
> > > @@ -0,0 +1,150 @@
> > > +/*
> > > + * Copyright (C) 2014 - Jonathan Rajotte <jonathan.r.julien at gmail.com>
> > > + * - Olivier Cotte <olivier.cotte at polymtl.ca>
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify it
> > > + * under the terms of the GNU General Public License, version 2 only, as
> > > + * published by the Free Software Foundation.
> > > + *
> > > + * This program is distributed in the hope that it will be useful, but WITHOUT
> > > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> > > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> > > + * more details.
> > > + *
> > > + * You should have received a copy of the GNU General Public License along with
> > > + * this program; if not, write to the Free Software Foundation, Inc., 51
> > > + * Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> > > + */
> > > +
> > > +#ifndef _MI_H
> > > +#define _MI_H
> > > +
> > > +#include <common/config/config.h>
> > > +#include <common/error.h>
> > > +#include <common/macros.h>
> > > +#include <stdint.h>
> > > +
> > > +/* Instance of a machine interface writer. */
> > > +typedef struct config_writer mi_writer;
> > > +
> > > +/*
> > > + * Create an instance of a machine interface writer.
> > > + *
> > > + * fd_output File to which the XML content must be written. The file will be
> > > + * closed once the mi_writer has been destroyed.
> > > + *
> > > + * Returns an instance of a machine interface writer on success, NULL on
> > > + * error.
> > > + */
> > > +LTTNG_HIDDEN
> > > +mi_writer *mi_writer_create(int fd_output);
> > > +
> > > +/*
> > > + * Destroy an instance of a configuration writer.
> > > + *
> > > + * writer An instance of a configuration writer.
> > > + *
> > > + * Returns zero if the XML document could be closed cleanly. Negative values
> > > + * indicate an error.
> > > + */
> > > +LTTNG_HIDDEN
> > > +int mi_writer_destroy(mi_writer *writer);
> > > +
> > > +
> > > +LTTNG_HIDDEN
> > > +int mi_writer_command_open(mi_writer *writer, const char* command);
> > > +
> > > +LTTNG_HIDDEN
> > > +int mi_writer_command_close(mi_writer *writer);
> > > +
> > > +/*
> > > + * Open an element tag.
> > > + *
> > > + * writer An instance of a configuration writer.
> > > + *
> > > + * element_name Element tag name.
> > > + *
> > > + * Returns zero if the XML document could be closed cleanly.
> > > + * Negative values indicate an error.
> > > + */
> > > +LTTNG_HIDDEN
> > > +int mi_writer_open_element(mi_writer *writer,
> > > + const char *element_name);
> > > +
> > > +/*
> > > + * Close the current element tag.
> > > + *
> > > + * writer An instance of a configuration writer.
> > > + *
> > > + * Returns zero if the XML document could be closed cleanly.
> > > + * Negative values indicate an error.
> > > + */
> > > +LTTNG_HIDDEN
> > > +int mi_writer_close_element(mi_writer *writer);
> > > +
> > > +/*
> > > + * Write an element of type unsigned int.
> > > + *
> > > + * writer An instance of a configuration writer.
> > > + *
> > > + * element_name Element name.
> > > + *
> > > + * value Unsigned int value of the element
> > > + *
> > > + * Returns zero if the element's value could be written.
> > > + * Negative values indicate an error.
> > > + */
> > > +LTTNG_HIDDEN
> > > +int mi_writer_write_element_unsigned_int(mi_writer *writer,
> > > + const char *element_name, uint64_t value);
> > > +
> > > +/*
> > > + * Write an element of type signed int.
> > > + *
> > > + * writer An instance of a configuration writer.
> > > + *
> > > + * element_name Element name.
> > > + *
> > > + * value Signed int value of the element
> > > + *
> > > + * Returns zero if the element's value could be written.
> > > + * Negative values indicate an error.
> > > + */
> > > +LTTNG_HIDDEN
> > > +int mi_writer_write_element_signed_int(mi_writer *writer,
> > > + const char *element_name, int64_t value);
> > > +
> > > +/*
> > > + * Write an element of type boolean.
> > > + *
> > > + * writer An instance of a configuration writer.
> > > + *
> > > + * element_name Element name.
> > > + *
> > > + * value Boolean value of the element
> > > + *
> > > + * Returns zero if the element's value could be written.
> > > + * Negative values indicate an error.
> > > + */
> > > +LTTNG_HIDDEN
> > > +int mi_writer_write_element_bool(mi_writer *writer,
> > > + const char *element_name, int value);
> > > +
> > > +/*
> > > + * Write an element of type string.
> > > + *
> > > + * writer An instance of a configuration writer.
> > > + *
> > > + * element_name Element name.
> > > + *
> > > + * value String value of the element
> > > + *
> > > + * Returns zero if the element's value could be written.
> > > + * Negative values indicate an error.
> > > + */
> > > +LTTNG_HIDDEN
> > > +int mi_writer_write_element_string(mi_writer *writer,
> > > + const char *element_name, const char *value);
> > > +
> > > +
> > > +#endif /* _MI_H */
> > > --
> > > 1.7.9.5
> > >
> >
> > 2014-03-07 14:28 GMT-05:00 Jonathan Rajotte <jonathan.r.julien at gmail.com>:
> > > Bonjour David, Jérémie
> > >
> > > Voici un prototype de MI pour la commande version et list -k. Nous avions
> > > plus cependant nous avons eu de la misère dans les deux derniers jours avec
> > > les test de régression ... mauvaise logistique de notre coté. Ces patchs
> > > sont basé sur la branche https://github.com/jgalar/lttng-tools save-restore
> > > de Jérémie.
> > >
> > > Cela peux sembler peux cependant nous avons essayer de vous proposer de quoi
> > > qui, on l'espère, s'incorpore dans lttng correctement.. Nous avons
> > > probablement tord mais bon ... On attend votre feedback.
> > >
> > > Concernant la gestion des erreurs, tel que discuté avec Jérémie, nous allons
> > > probablement output le mi dans un fichiers unique temporaire puis le
> > > rediriger vers stdout pour éviter du garbage en output. Je travaille
> > > activement sur cela en fds.
> > >
> > > Cependant vous comprendrez que avant d'aller plus loin, on voudrait que
> > > notre base soit valide. Nous sommes conscient de notre retard concernant
> > > cela. On prévoit mettre les bouchés triples.
> > >
> > > Dans l'éventualité que le travail effectué ne vous convient absolument pas,
> > > j'aimerais que l'on puisse se rencontrer en personne/hangout/skype/appel
> > > conférence pour remédier rapidement à la situation plutôt que de faire cela
> > > via un échange d'email et de irc... Je suis conscient de la valeur de votre
> > > temps cependant je crois que cela réglerais le problème plus rapidement.
> > >
> > > Voilà, on attend de vos nouvelles.
> > >
> > > --
> > > Jonathan Rajotte Julien
> > > Chargé de laboratoire, INF1995
> > > Polytechnique Montréal
> >
> >
> >
> > --
> > Jérémie Galarneau
> > EfficiOS Inc.
> > http://www.efficios.com
--
Jonathan Rajotte Julien
Chargé de laboratoire INF1995
Membre MD6
Polytechnique Montréal
More information about the lttng-dev
mailing list