Re: [Xen-devel] [OSSTEST PATCH v12 18/21] TestSupport: Implement target_cmd_subunit a subunit stream parser into substeps

[Ian Jackson <ian.jackson@xxxxxxxxxxxxx>]

Anthony PERARD writes ("Re: [OSSTEST PATCH v12 18/21] TestSupport: Implement 
target_cmd_subunit a subunit stream parser into substeps"):
> On Thu, Jul 13, 2017 at 02:28:11PM +0100, Ian Jackson wrote:
> > I think this is not a critical problem, but fixing it would be nice at
> > some point.
> 
> The subunit stream contains the timestamps, so it just a matter of
> having substep_* taking it as an argument.

Ah.  Well, that can wait.

> > > +sub subunit_sanitize ($) {
> > > +    my ($testname) = @_;
> > > +    $testname =~ s/ /_/g;
> > > +    return $testname;
> > > +}
> > 
> > This function should have a more specific name.  Also it needs to be a
> > whitelist.
> 
> What about subunit_sanitize_testname?

SGTM.

> What kind of whitelist? What should it includes?

I mean: currently, you replace spaces with _s.  But you ought to have
a specific list of characters that you allow without replacing.

It doesn't help that osstest doesn't have an official character set
for testids.  Existing testids (other than various junk) contain
all of
    [^;_.()/~0-9a-zA-Z-]
(in Perl regexp charset syntax).

I suggest permitting all of those except ; which seems to have been a
mistake and has now been eliminated.  You may want to permit [ ] too.

> > Does the subunit protocol insist that
> > the spaces are single spaces ?  If not you need to use \s+.  You may
> > want to use the extended regexp syntax.
> 
> Looking at a description of the protocol and at the subunit code, does
> are single spaces.

Fair enough.

> What do you mean by "extended" ? Maybe operator like /.+?/, or maybe
> /(?<NAME>pattern)/ ?

I mean //x.  See "/x" in perlre(1).  This is a matter of taste.

> multipart just describes how the following lines are formated, it would
> have the 'content-type:' and the size of the output. non-multipart is
> just followed by text, and ends with '\n]\n' (both format do).
> 
> I don't think the code needs to care about it, just that it may or
> may not be there.

Hrm.

> > Does the subunit protocol not have any escaping ?  (Ie, what happens
> > if a thing run as part of a subunit test actually generates a line of
> > log output "]" ?)  If it does havve some escaping you need to
> > de-escape it.
> 
> Without "multipart", there does not seems to be any escaping. With
> multipart, the size of the output is in the protocol, I could extend the
> parser take it into account. It's just more work.

I think you must be able to consume the "multipart" and use the size,
then.  Otherwise if a test case printed output containing "\n]\n", in
multipart format, you would mishandle it and get desynchronised.

> FYI, part of the protocol about the output (with the beginning of
> DETAILS been "\[( multipart)?" in the regex):
> 
> DETAILS ::= BRACKETED | MULTIPART
> BRACKETED ::= '[' CR UTF8-lines ']' CR
> MULTIPART ::= '[ multipart' CR PART* ']' CR
> PART ::= PART_TYPE CR NAME CR PART_BYTES CR
> PART_TYPE ::= Content-Type: type/sub-type(;parameter=value,parameter=value)
> PART_BYTES ::= (DIGITS CR LF BYTE{DIGITS})* '0' CR LF

Sounds like you have to parse the multipart counted parts, I'm afraid.

> > What are subunit v1 consumers supposed to do with 1. unknown keywords
> > 2. syntax errors ?
> > I doubt that the answer to (2) is to ignore them as you do here...
> 
> "unexpected lines [...] should be forwarded unaltered". That's is in the
> readme of python-subunit project.

"forwarded" where I wonder ?  I guess printing them to logm is fine.
But you shouldn't ignore them, I think.

> As for keywords that can exist, there is "tags:", but in the case of
> tempest, it describe which worker did a test, when there is several
> concurrent worker. There is also "progress:" which is not very usefull
> for osstest. There is maybe more keywords which are test result which I
> should probably find out what there are, but I've got at least the one
> used by Tempest.

Fair enough.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel