Re: [PATCH 1/2] dynamic_debug: fix boot parameters parse

2015-10-28 Thread Peter Chen
On Wed, Oct 28, 2015 at 10:17:59AM -0400, Jason Baron wrote:
> On 10/27/2015 03:40 AM, Peter Chen wrote:
> > The parse_args will delete space between boot parameters, so
> > if we add dyndbg="file drivers/usb/* +p" at bootargs, the parse_args
> > will split it as three parameters, and only "file" is for dyndbg,
> > then below error will occur at ddebug, it causes all non-module
> > ddebug fail during the boot process.
> > 
> > ===
> > dynamic_debug:ddebug_parse_flags: bad flag-op f, at start of file
> > dynamic_debug:ddebug_exec_query: flags parse failed
> > ===
> > 
> 
> Hi,
> 
> So the spaces there should be preserved if they are in quotes. Looking at
> kernel/params.c:next_arg(), the comment there explicitly says:
> 
> /* You can use " around spaces, but can't escape ". */
> 
> So this should still be working. And if its not, I suspect other
> command-line parsing would be broken as well. So if it did change,
> what changed in command line parsing?
> 
> What's the output of $ cat /proc/cmdline ? Perhaps, the quotes
> were not passed properly from the bootloader to the kernel?
> 
> Thanks,
> 
> -Jason

Oh, Jason. skip this patch pleaes, the u-boot needs to use "\\" below
double quoting to setenv. eg: dyndbg="\\"file drivers/usb/chipidea/*+p"\\",
then, the cmdline will like: dyndbg="file drivers/usb/chipidea/* +p",
otherwise, there is no double quoting around QUERY.

Peter
> 
> 
> > As a solution, we can use comma to split parameters for ddebug booting
> > parameter, and replace comma with space at code, in that case, the ddebug
> > core can handle it with the same way we do it for /sys.
> > 
> > Signed-off-by: Peter Chen 
> > Cc: 
> > ---
> >  lib/dynamic_debug.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> > index e491e02..da4883b 100644
> > --- a/lib/dynamic_debug.c
> > +++ b/lib/dynamic_debug.c
> > @@ -880,6 +880,8 @@ static int ddebug_dyndbg_param_cb(char *param, char 
> > *val,
> > if (strcmp(param, "dyndbg"))
> > return on_err; /* determined by caller */
> >  
> > +   /* relace comma with space */
> > +   strreplace(val, ',', ' ');
> > ddebug_exec_queries((val ? val : "+p"), modname);
> >  
> > return 0; /* query failure shouldnt stop module load */
> > 
> 

-- 

Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH 1/2] dynamic_debug: fix boot parameters parse

2015-10-28 Thread Jason Baron
On 10/27/2015 03:40 AM, Peter Chen wrote:
> The parse_args will delete space between boot parameters, so
> if we add dyndbg="file drivers/usb/* +p" at bootargs, the parse_args
> will split it as three parameters, and only "file" is for dyndbg,
> then below error will occur at ddebug, it causes all non-module
> ddebug fail during the boot process.
> 
> ===
> dynamic_debug:ddebug_parse_flags: bad flag-op f, at start of file
> dynamic_debug:ddebug_exec_query: flags parse failed
> ===
> 

Hi,

So the spaces there should be preserved if they are in quotes. Looking at
kernel/params.c:next_arg(), the comment there explicitly says:

/* You can use " around spaces, but can't escape ". */

So this should still be working. And if its not, I suspect other
command-line parsing would be broken as well. So if it did change,
what changed in command line parsing?

What's the output of $ cat /proc/cmdline ? Perhaps, the quotes
were not passed properly from the bootloader to the kernel?

Thanks,

-Jason


> As a solution, we can use comma to split parameters for ddebug booting
> parameter, and replace comma with space at code, in that case, the ddebug
> core can handle it with the same way we do it for /sys.
> 
> Signed-off-by: Peter Chen 
> Cc: 
> ---
>  lib/dynamic_debug.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index e491e02..da4883b 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -880,6 +880,8 @@ static int ddebug_dyndbg_param_cb(char *param, char *val,
>   if (strcmp(param, "dyndbg"))
>   return on_err; /* determined by caller */
>  
> + /* relace comma with space */
> + strreplace(val, ',', ' ');
>   ddebug_exec_queries((val ? val : "+p"), modname);
>  
>   return 0; /* query failure shouldnt stop module load */
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


[PATCH 1/2] dynamic_debug: fix boot parameters parse

2015-10-27 Thread Peter Chen
The parse_args will delete space between boot parameters, so
if we add dyndbg="file drivers/usb/* +p" at bootargs, the parse_args
will split it as three parameters, and only "file" is for dyndbg,
then below error will occur at ddebug, it causes all non-module
ddebug fail during the boot process.

===
dynamic_debug:ddebug_parse_flags: bad flag-op f, at start of file
dynamic_debug:ddebug_exec_query: flags parse failed
===

As a solution, we can use comma to split parameters for ddebug booting
parameter, and replace comma with space at code, in that case, the ddebug
core can handle it with the same way we do it for /sys.

Signed-off-by: Peter Chen 
Cc: 
---
 lib/dynamic_debug.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index e491e02..da4883b 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -880,6 +880,8 @@ static int ddebug_dyndbg_param_cb(char *param, char *val,
if (strcmp(param, "dyndbg"))
return on_err; /* determined by caller */
 
+   /* relace comma with space */
+   strreplace(val, ',', ' ');
ddebug_exec_queries((val ? val : "+p"), modname);
 
return 0; /* query failure shouldnt stop module load */
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/