Re: [Xen-devel] [PATCH v3] ns16550-Add-command-line-parsing-adjustments

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.04.17 at 20:47, <sparatey@xxxxxxx> wrote:
> On 4/3/2017 6:55 AM, Jan Beulich wrote:
>>>>> On 31.03.17 at 17:42,<swapnil.paratey@xxxxxxx>  wrote:
>> The title needs improvement - it doesn't really reflect what the
>> patch does.
> 
> I apologize for this. I kept the name same since it was the third version
> of the patch and thought that it would help in tracking the evolution of
> the patch.
> 
> The new proposed title:
> - ns16550: Support for UART parameters to be specified as name-value pairs

Sounds good (albeit it sounds like you don't want to do the
requested splitting).

> Please let me know about
> - how to track the evolution of the patch (whether to mention [PATCH v4])

Definitely v4, yes.

> - whether this patch should mention ChangeSets from the previous versions

At least changed from v3 to v4 should be described, including the
change of title.

> - if the subject should be more precise and whether I should respect Git's
> 60-character recommended limit for patch subjects.

I don't think we normally pay particular attention to this, but it
doesn't hurt to not have overly long titles.

>>> --- a/xen/common/kernel.c
>>> +++ b/xen/common/kernel.c
>>> @@ -48,7 +48,7 @@ static void __init assign_integer_param(
>>>   
>>>   void __init cmdline_parse(const char *cmdline)
>>>   {
>>> -    char opt[100], *optval, *optkey, *q;
>>> +    char opt[512], *optval, *optkey, *q;
>> Why not MAX_CMDLINE_LENGTH? But anyway both this and ...
>>
>>> --- a/xen/drivers/char/ns16550.c
>>> +++ b/xen/drivers/char/ns16550.c
>>> @@ -38,11 +38,27 @@
>>>    * can be specified in place of a numeric baud rate. Polled mode is 
>>> specified
>>>    * by requesting irq 0.
>>>    */
>>> -static char __initdata opt_com1[30] = "";
>>> -static char __initdata opt_com2[30] = "";
>>> +static char __initdata opt_com1[MAX_CMDLINE_LENGTH] = "";
>>> +static char __initdata opt_com2[MAX_CMDLINE_LENGTH] = "";
>> ... this seems to be excessive growth.
> 
> Is 128 bytes a good number? It easily accommodates all the UART command line
> parameters.

Being able to fit all sub-options is indeed the primary criteria. If 128
fulfills this without being excessively much larger, the number should
be fine.

>>> -static void __init ns16550_parse_port_config(
>>> -    struct ns16550 *uart, const char *conf)
>>> +#define PARSE_ERR_RET(_f, _a...)             \
>>> +    do {                                     \
>>> +        printk( "ERROR: " _f "\n" , ## _a ); \
>>> +        return 1;                            \
>>> +    } while ( 0 )
>>> +
>>> +
>>> +int parse_positional(struct ns16550 *uart, char **str)
>> static ... __init
>>
>> Why is the return type of this function not void? All return
>> statements uniformly return zero.
> 
> There are two places where PARSE_ERR_RET has been used and it is
> returning 1 instead of 0 (to exit the ns16550_parse_port_config
> function if the parsing wasn't correct). This is used to maintain
> previous behavior - if the parsing for pci is not correct, it
> should exit the function without going through config_parsed.

Ah, I see. I think nowadays we'd prefer to avoid such hidden return
(or other control flow) statements, unless that unduly uglifies the
code.

>>> @@ -1165,11 +1248,122 @@ static void __init ns16550_parse_port_config(
>>>       {
>>>           if ( !parse_pci(conf, NULL, &uart->pb_bdf[0],
>>>                           &uart->pb_bdf[1], &uart->pb_bdf[2]) )
>>> -            PARSE_ERR("Bad bridge PCI coordinates");
>>> +            PARSE_ERR_RET("Bad bridge PCI coordinates");
>>>           uart->pb_bdf_enable = 1;
>>>       }
>>>   #endif
>>>   
>>> +    return 0;
>>> +}
>>> +
>>> +int parse_namevalue_pairs(char *str, struct ns16550 *uart)
>> static ... __init
>>
>> Looking at the return values, this perhaps would better return bool.
> 
> This is based on the understanding that (return 0) implies a successful
> return from a function whereas a non-zero value means an error.
> 
> If we use true/false bools, should I end the function with (return false)
> for a successful return OR should I use (return true) with
> if ( !parse_namevalue_pairs) to check if the function returned SUCCESS?
> 
> The aim of using int was to ensure readability and understanding
> successful returns from a function.

This is a rather bogus argument imo: Boolean return values are in
no way worse. The common expectation to functions returning int
is that they can return actual values (perhaps -E... error codes).
To merely indicate success or failure, using true/false respectively
is quite common an approach.

>>> +            case IO_BASE:
>>> +                if (dev_set == 1)
>>> +                    PARSE_ERR_RET("Can't use io_base with dev=pci or 
>>> dev=amt options\n");
>> Doesn't this apply the other way around then too?
> 
> In current code behavior, mentioning pci in the command line sets the io_base
> automatically. We cannot specify io_base and pci simultaneously in the
> same line with the current code. Using name-value pairs gives the user a
> chance to specify both of these options together.
> 
> For example, a user can write "dev=pci,io_base=0xfedca000" OR
> "io_base=0xfedca000,dev=pci"
> 
> In the case where a user has mentioned pci and then set the io_base too,
> the new user-specified io_base option will be written over the pci-probed
> io_base which will render the PCI UART device without an io_base and
> hence unusable - since we have the wrong io_base for the PCI device.
> This is why I have included the message for this condition.
> 
> However, if a user wants to write the io_base and then specify pci,
> the pci_uart_config function will happily overwrite the io_base without
> the user knowing that the io_base has been written over by the pci-probing
> code (pci_uart_config) and the PCI UART device will work (provided
> pci_uart_config succeeds in setting up the PCI UART device).
> 
> Hence, only the first case has been addressed here - to prevent the user from
> over-writing over their own pci settings. The other way around doesn't
> stop the user from receiving console messages.
> 
> Please let me know if this behavior is acceptable or something has to 
> change.

I would think that my question indicated pretty clearly my position
on this: Silently ignoring user settings is almost as undesirable as
conflicting settings leading to misbehavior. Following your reply it
may indeed not be necessary to error out in this case, but a
warning should still be issued imo.

>>> +static void __init ns16550_parse_port_config(
>>> +    struct ns16550 *uart, const char *conf)
>>> +{
>>> +    char cmdline[MAX_CMDLINE_LENGTH];
>> This isn't the entire cmdline, is it?
> 
> This is only the UART command line options. For example,
> when we specify
> com1=115200,8n1,0x3f8,4
> cmdline is only "115200,8n1,0x3f8,4" without the "com1="
> 
> However, the opt buffer (which I have also set to 512 - switching to 128 
> now),
> includes the 5 characters of "com1=". This buffer is in the cmdline_parse
> function in xen/common/kernel.c
> 
> Should this "com1=" case be handled? How should the buffer lengths be set 
> here?
> 
> Previous code had 30 character buffer for both the buffers (despite one 
> holding
> 5 more characters than the other).

I think this level of detail that you're asking for goes too far - you
should really work this out yourself (after all, as long as you can
explain _why_ you used a certain size, and as long as that
explanation can be followed, it's not like such choices can't be
left to your own judgment; you don't need reviewer feedback on
every nitty gritty detail). Buffers should be large enough to
accommodate all reasonable input, and they shouldn't be
excessively much larger than that. I don't think an overallocation
of 5 characters would come anywhere near "excessive".

>>> --- a/xen/include/xen/serial.h
>>> +++ b/xen/include/xen/serial.h
>>> @@ -20,6 +20,8 @@ void serial_set_rx_handler(int handle, serial_rx_fn fn);
>>>   
>>>   /* Number of characters we buffer for a polling receiver. */
>>>   #define serial_rxbufsz 32
>>> +#define MAX_CMDLINE_LENGTH 512
>>> +#define MAX_PARAM_VALUE_LENGTH 16
>> Please don't put constants needed in only a single source file into
>> a header, even less so with such generic names.
> Buffer lengths are used in two files
> - xen/common/kernel.c - cmdline_parse - opt buffer
> (5 extra characters than cmdline mentioned in the next line)
> - xen/drivers/char/ns16550.c - ns16550_parse_port_config - cmdline
> 
> Should these lengths be mentioned somewhere common OR an association be 
> created
> between them? I spent some time understanding why my code was eating up 5
> characters out of nowhere ("com1=") when execution passed between these two.
> If it was linked through a #define or a common macro, this might have be 
> avoided.

Judging by your patch, both were used only in ns16550.c. Hence
besides the names being too generic for that purpose, they would
belong into that file. If there is a connection to be made to
kernel.c, then I can see value in adding such constants, but in a
header suitable for their names and purposes. For example I don't
consider it reasonable for kernel.c to include serial.h - it should
have no need to know of serial driver specifics.

Jan

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