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

[Yuri Volchkov <yuri.volchkov@xxxxxxxxx>]

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


>
>> +    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.

-- 
Yuri Volchkov
Software Specialist

NEC Europe Ltd
Kurfürsten-Anlage 36
D-69115 Heidelberg

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