Re: [PATCH 19/20] tools/xenstore: introduce trace classes

[Juergen Gross <jgross@xxxxxxxx>]

On 06.11.22 23:18, Julien Grall wrote:
> Hi Juergen,
>
> I haven't yet look at the code itself. I wanted to comment on the external 
> interfaces.
>
> On 01/11/2022 15:28, Juergen Gross wrote:
> > Make the xenstored internal trace configurable by adding classes
> > which can be switched on and off independently from each other.
> >
> > Define the following classes:
> >
> > - obj: Creation and deletion of interesting "objects" (watch,
> >    transaction, connection)
> > - io: incoming requests and outgoing responses
> > - wrl: write limiting
> >
> > Per default "obj" and "io" are switched on.
> >
> > Entries written via trace() will always be printed (if tracing is on
> > at all).
> >
> > Rename the misnamed xenstore-control commands "logfile" to "tracefile"
> > and "log" to "trace".
> While I understand this may be misnamed, this also means that there is an ABI 
> breakage between current Xenstored and the future one.
> I am not convinced this is justified. Therefore, I think we should at minimum 
> keep the compatibility.
Hmm, I can see your point. Given that this might be one of the most
common used xenstore-control commands I'm not opposed to keep the current
names.

> > Add the capability to control the trace settings via the "trace"
> > command and via a new "--trace-control" command line option.
> >
> > Add a missing trace_create() call for creating a transaction.
> >
> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > ---
> >   docs/misc/xenstore.txt                 | 18 +++++++----
> >   tools/xenstore/xenstored_control.c     | 41 +++++++++++++++++++-----
> >   tools/xenstore/xenstored_core.c        | 44 +++++++++++++++++++++++---
> >   tools/xenstore/xenstored_core.h        |  6 ++++
> >   tools/xenstore/xenstored_domain.c      | 27 ++++++++--------
> >   tools/xenstore/xenstored_transaction.c |  1 +
> >   6 files changed, 105 insertions(+), 32 deletions(-)
> >
> > diff --git a/docs/misc/xenstore.txt b/docs/misc/xenstore.txt
> > index 44428ae3a7..9db0385120 100644
> > --- a/docs/misc/xenstore.txt
> > +++ b/docs/misc/xenstore.txt
> > @@ -409,14 +409,8 @@ CONTROL            <command>|[<parameters>|]
> >           error string in case of failure. -s can return "BUSY" in case
> >           of an active transaction, a retry of -s can be done in that
> >           case.
> > -    log|on
> > -        turn xenstore logging on
> > -    log|off
> > -        turn xenstore logging off
> > -    logfile|<file-name>
> > -        log to specified file
> Technically xenstore.txt is meant to describe an interface that is 
> implementation agnostics. Can you outline in the documentation why removing them 
> is fine?
> >       memreport|[<file-name>]
> > -        print memory statistics to logfile (no <file-name>
> > +        print memory statistics to tracefile (no <file-name>
> >           specified) or to specific file
> >       print|<string>
> >           print <string> to syslog (xenstore runs as daemon) or
> > @@ -432,6 +426,16 @@ CONTROL            <command>|[<parameters>|]
> >           the domain <domid>
> >       quota-soft|[set <name> <val>]
> >           like the "quota" command, but for soft-quota.
> > +    trace|[+|-<switch>]
> The regex here is a bit ambiguous because it technically means either "+" or 
> "-<switch>". Furthermore, within this docs, there are case where | is included 
> in between [] to indicate the this is terminated by a \0.
> So it would be better to define it over 3 lines:
>
> trace
> trace|+<switch>
> trace|-<switch>
>
> I think it would be fine if there is only one paragraph of explanation for the 
> 3.
Okay.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature