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

[Julien Grall <julien@xxxxxxx>]

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.
> 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.
> +               without parameters: show possible trace switches
> +               +<switch> activates trace entries for <switch>,
> +               -<switch> deactivates trace entries for <switch>
> +       trace|on
> +               turn xenstore tracing on
> +       trace|off
> +               turn xenstore tracing off
> +       tracefile|<file-name>
> +               trace to specified file
>         help                    <supported-commands>
>                 return list of supported commands for CONTROL
>   
Cheers,

--
Julien Grall