Re: [Xen-devel] [v7][PATCH 16/16] tools: parse to enable new rdm policy parameters

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Chen, Tiejun writes ("Re: [v7][PATCH 16/16] tools: parse to enable new rdm 
policy parameters"):
> > The first issue (which would really be relevant to the documentation
> > patch) is that the documentation is in a separate commit.  There are
> > sometimes valid reasons for doing this.  I'm not sure if they apply,
> 
> Wei suggested we should organize/spit all patches according to libxl, 
> libxc, xc and xl.

Yes.  I don't want to put you in the middle of a
disagreement/misunderstanding between Wei and me.  This is in any case
a fairly minor issue.

> In this patch head description, maybe I can change something like this
> 
> This patch parses to enable user configurable parameters to specify
> RDM resource and according policies which are defined previously,

That would be good, yes, thanks.

> >> +                }else if ( !strcmp(optkey, "rdm_policy") ) {
> >> +                    if ( !strcmp(tok, "strict") ) {
> >> +                        pcidev->rdm_policy = 
> >> LIBXL_RDM_RESERVE_POLICY_STRICT;
> >> +                    } else if ( !strcmp(tok, "relaxed") ) {
> >> +                        pcidev->rdm_policy = 
> >> LIBXL_RDM_RESERVE_POLICY_RELAXED;
> >> +                    } else {
> >> +                        XLU__PCI_ERR(cfg, "%s is not an valid PCI RDM 
> >> property"
> >> +                                          " policy: 'strict' or 
> >> 'relaxed'.",
> >> +                                     tok);
> >> +                        goto parse_error;
> >> +                    }
> >
> > This section has coding style (whitespace) problems and long lines.
> > If you need to respin, please fix them.
> 
> Are you saying this?
> 
> } else if (  -> }else if (
> } else { -> }else {

Also spurious spaces inside brackets.  Please see CODING_STYLE.

> Additionally I don't found which line is over 80 characters.

Hmm, I see that the longest line is exactly 80.

Of course 80 characters, plus a `+' from a patch, and `>' for email
quoting, makes more than 80: so it has wrap damage on my screen.  You
are right that this conforms to the wording in the style, which says
`75-80'.  So I won't insist on this change (unless I manage to
persuade my comaintainers to strenghten this restriction in the style
guide).

> >> +    for (tok = ptr, end = ptr + strlen(ptr) + 1; ptr < end; ptr++) {
> >> +        switch(state) {
> >> +        case STATE_TYPE:
> >
> > This code is extremely repetitive.
> 
> I just refer to xlu_pci_parse_bdf()

Yes.  I'm afraid that xlu_pci_parse_bdf is a poor example.

> > Really I would prefer that this parsing was done with a miniature flex
> > parser, rather than ad-hoc pointer arithmetic and use of strtok.
> 
> Sorry, could you show this explicitly?

Something like what was done for disk devices.  See libxlu_disk_l.l
for an example.  In this case your code would be a lot less
complicated than what you see there.

After the codefreeze I would probably have some time to write it for
you.  (I think that would be valuable because libxlu_disk_l.l is a
very complicated example, and I want be able to point future
submitters at something simpler.)

Ian.

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