Re: [Minios-devel] [UNIKRAFT PATCH 6/8] support/scripts: implement trace buffer parcing

[Costin Lupu <costin.lup@xxxxxxxxx>]

On 5/29/19 4:42 PM, Yuri Volchkov wrote:
> Hi,
> 
> Costin Lupu <costin.lup@xxxxxxxxx> writes:
> 
>> Hi Yuri,
>>
>> In the subject: s/parcing/parsing .
> Ack
> 
>>
>> Please see my comments inline.
>>
>> On 5/10/19 9:29 PM, Yuri Volchkov wrote:
>>> With this patch trace.py can print the data collected in the
>>> tracefile. And two more use cases are covered for convince:
>>
>> s/for convince/for convenience/
> Ack
> 
>>
>>> +
>>> +class tp_sample:
>>
>> It can get confusing on why there are 2 classes for tracepoints,
>> tracepoint and tp_sample. I think their names should reflect that the
>> former are the tracepoint definitions and latter are the tracepoint values.
> Hm, I thought that tracepoint sample is a lot more clear that is a
> _sample_ of a tracepoint, than "tracepoint value". From merriam-webster
> dictionary (https://www.merriam-webster.com/dictionary/sample):
> 
>     1) a representative part or a single item from a larger whole or
>        group especially when presented for inspection or shown as
>        evidence of quality
>     
>     2) finite part of a statistical population whose properties are
>        studied to gain information about the whole

Oh, and I thought that one of the purposes of reviewing was to help the
author with suggestions in order to make the code even more clearer, you
know, for other people than the author.

> 
> 
>>
>>> +    def __init__(self, tp, time, args):
>>> +        self.tp = tp
>>> +        self.args = args
>>> +        self.time = time
>>> +    def __str__(self):
>>> +        return (("%016d %s: " % (self.time, self.tp.name)) +
>>> +                 (self.tp.fmt % self.args))
>>> +    def tabulate_fmt(self):
>>> +        return [self.time, self.tp.name, (self.tp.fmt % self.args)]
>>> +
>>> +class EndOfBuffer(Exception):
>>> +    pass
>>> +
>>> +# Parsing of trace buffer is designed to be used without gdb. If ever
>>> +# Unikraft will have a bare metal port, it would be complicated to use
>>> +# gdb for fetching and parsing runtime data.
>>> +#
>>> +# However, it is possible anyways to get an address of the statically
>>> +# allocated trace buffer using gdb/nm/objdump. And we can use some
>>> +# different mechanism to copy memory from the running instance of
>>> +# Unikraft (which is yet to be designed).
>>> +#
>>> +# Similar considerations are applied to parsing trace point
>>> +# definitions. We can do that disregarding if it is possible to attach
>>> +# gdb to a running instance or not
>>> +class sample_parser:
>>> +    def __init__(self, keyvals, tp_defs_data, trace_buff, ptr_size):
>>> +        if (int(keyvals['format_version']) > FORMAT_VERSION):
>>> +            print("Warning: Version of trace format is more recent",
>>> +                  file=sys.stderr)
>>> +        self.data = unpacker(trace_buff)
>>> +        self.tps = get_tp_definitions(tp_defs_data, ptr_size)
>>> +    def __iter__(self):
>>> +        self.data.pos = 0
>>> +        return self
>>> +    def __next__(self):
>>> +        try:
>>> +            # TODO: generate format. Cookie can be 4 bytes long on other
>>> +            # platforms
>>> +            magic,size,time,cookie = self.data.unpack('4sLQQ')
>>> +        except EndOfBuffer:
>>> +            raise StopIteration
>>> +
>>> +        magic = magic.decode()
>>> +        if (magic != TP_HEADER_MAGIC):
>>> +            raise StopIteration
>>> +
>>> +        tp = self.tps[cookie]
>>> +        args = []
>>> +        for i in range(tp.args_nr):
>>> +            if tp.types[i] == UK_TRACE_ARG_STRING:
>>> +                args += [self.data.unpack_string()]
>>> +            else:
>>> +                args += [self.data.unpack_int(tp.sizes[i])]
>>> +
>>> +        return tp_sample(tp, time, tuple(args))
>>> +
>>
>> I think it would have been clearer if the parsing alone would have
>> stayed in its own module. Or at least delimit the sections in the module
>> with (block) comments.
> What do you mean in it's own module? It is in it's own module?
> Everything in this file is about parsing.
> 

The changes in 'support/scripts: reorganise tracing scripts' (btw, that
title has a type) would clarify that.

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel