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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Tiejun Chen writes ("[v7][PATCH 16/16] tools: parse to enable new rdm policy 
parameters"):
> This patch parses to enable user configurable parameters to specify
> RDM resource and according policies,
> 
> Global RDM parameter:
>     rdm = "strategy=host,policy=strict/relaxed"
> Per-device RDM parameter:
>     pci = [ 'sbdf, rdm_policy=strict/relaxed' ]
> 
> Default per-device RDM policy is same as default global RDM policy as being
> 'relaxed'. And the per-device policy would override the global policy like
> others.

Thanks for this.  I have found a couple of things in this patch which
I would like to see improved.  See below.

Again, given how late I am, I do not feel that I should be nacking it
at this time.  You have a tools ack from Wei, so my comments are not a
blocker for this series.

But if you need to respin, please take these comments into account,
and consider which are feasible to fix in the time available.  If you
are respinning this series targeting Xen 4.7 or later, please address
all of the points I make below.

Thanks.


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,
but if they do this should be explained in one of the commit
messages.  If this was done I'm afraid I have missed it.

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

> +    for (tok = ptr, end = ptr + strlen(ptr) + 1; ptr < end; ptr++) {
> +        switch(state) {
> +        case STATE_TYPE:
> +            if (*ptr == '=') {
> +                state = STATE_RDM_STRATEGY;
> +                *ptr = '\0';
> +                if (strcmp(tok, "strategy")) {
> +                    XLU__PCI_ERR(cfg, "Unknown RDM state option: %s", tok);
> +                    goto parse_error;
> +                }
> +                tok = ptr + 1;
> +            }

This code is extremely repetitive.

Really I would prefer that this parsing was done with a miniature flex
parser, rather than ad-hoc pointer arithmetic and use of strtok.

Thanks,
Ian.

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