Re: [Xen-devel] [PATCH v5] ns16550: Add support for UART parameters to be specifed with name-value pairs

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.04.17 at 20:57, <swapnil.paratey@xxxxxxx> wrote:
> Changed since v4:
>  * Changes from [PATCH v4] code review

I'm sorry, but this is not enough.

> @@ -77,6 +94,30 @@ static struct ns16550 {
>  #endif
>  } ns16550_com[2] = { { 0 } };
>  
> +struct serial_param_var {
> +    const char name[12];

Pointless const.

> +static enum __init serial_param_type get_token(char *token, char *ext_value)

I don't see the need for the ext_ prefix.

> +{
> +    const char *param_name;
> +    unsigned int i;
> +
> +    param_name = strsep(&token, "=");
> +    if ( param_name == NULL )
> +        return num_serial_params;
> +
> +    /* Linear search for the parameter. */
> +    for ( i = 0; i < ARRAY_SIZE(sp_vars); i++ )
> +    {
> +        if ( strcmp(sp_vars[i].name, param_name) == 0 )
> +        {
> +            /*
> +             * Token has the param value after the equal to sign.
> +             * ext_value is a 16-byte buffer - param_value[16].
> +             */
> +            strlcpy(ext_value, token, 16);

So why do you copy here, instead of simply passing the pointer
back? This is the more that the caller didn't tell you it 16 bytes of
space allocated for the output value.

> +static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart)
> +{
> +    char *token, *start = str;
> +    char param_value[16];
> +    bool dev_set = false;
> +
> +    if ( (str == NULL) || (*str == '\0') )
> +        return true;
> +
> +    do
> +    {
> +        /* When no tokens are found, start will be NULL */
> +        token = strsep(&start, ",");
> +
> +        switch( get_token(token, param_value) ) {

Coding style (missing blank and brace on its own line).

> +        case baud:
> +            uart->baud = simple_strtoul(param_value, NULL, 0);
> +            break;
> +
> +        case bridge_bdf:
> +            if ( !parse_pci(param_value, NULL, &uart->ps_bdf[0],
> +                            &uart->ps_bdf[1], &uart->ps_bdf[2]) )
> +                PARSE_ERR_RET("Bad port PCI coordinates\n");
> +            uart->ps_bdf_enable = 1;
> +            break;
> +
> +        case clock_hz:
> +            uart->clock_hz = (simple_strtoul(param_value, NULL, 0) << 4);

Pointless parentheses.

Jan


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