[PATCH for-4.14] xen/hypfs: fix loglvl parameter setting

Juergen Gross posted 1 patch 3 years, 9 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/xen tags/patchew/20200609154546.4531-1-jgross@suse.com
Maintainers: Andrew Cooper <andrew.cooper3@citrix.com>, Ian Jackson <ian.jackson@eu.citrix.com>, George Dunlap <george.dunlap@citrix.com>, Jan Beulich <jbeulich@suse.com>, Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>, Julien Grall <julien@xen.org>
xen/drivers/char/console.c | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
[PATCH for-4.14] xen/hypfs: fix loglvl parameter setting
Posted by Juergen Gross 3 years, 9 months ago
Writing the runtime parameters loglvl or guest_loglvl omits setting the
new length of the resulting parameter value.

Reported-by: George Dunlap <george.dunlap@citrix.com>
Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/drivers/char/console.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 56e24821b2..861ad53a8f 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -241,14 +241,25 @@ static int _parse_loglvl(const char *s, int *lower, int *upper, char *val)
 
 static int parse_loglvl(const char *s)
 {
-    return _parse_loglvl(s, &xenlog_lower_thresh, &xenlog_upper_thresh,
-                         xenlog_val);
+    int ret;
+
+    ret = _parse_loglvl(s, &xenlog_lower_thresh, &xenlog_upper_thresh,
+                        xenlog_val);
+    custom_runtime_set_var(param_2_parfs(parse_loglvl), xenlog_val);
+
+    return ret;
 }
 
 static int parse_guest_loglvl(const char *s)
 {
-    return _parse_loglvl(s, &xenlog_guest_lower_thresh,
-                         &xenlog_guest_upper_thresh, xenlog_guest_val);
+    int ret;
+
+    ret = _parse_loglvl(s, &xenlog_guest_lower_thresh,
+                        &xenlog_guest_upper_thresh, xenlog_guest_val);
+    custom_runtime_set_var(param_2_parfs(parse_guest_loglvl),
+                           xenlog_guest_val);
+
+    return ret;
 }
 
 static char *loglvl_str(int lvl)
-- 
2.26.2


Re: [PATCH for-4.14] xen/hypfs: fix loglvl parameter setting
Posted by Julien Grall 3 years, 9 months ago
Hi Juergen,

On 09/06/2020 16:45, Juergen Gross wrote:
> Writing the runtime parameters loglvl or guest_loglvl omits setting the
> new length of the resulting parameter value.
> 
> Reported-by: George Dunlap <george.dunlap@citrix.com>
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Julien Grall <jgrall@amazon.com>

Although one unrelated comment below.

> ---
>   xen/drivers/char/console.c | 19 +++++++++++++++----
>   1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> index 56e24821b2..861ad53a8f 100644
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -241,14 +241,25 @@ static int _parse_loglvl(const char *s, int *lower, int *upper, char *val)
>   
>   static int parse_loglvl(const char *s)
>   {
> -    return _parse_loglvl(s, &xenlog_lower_thresh, &xenlog_upper_thresh,
> -                         xenlog_val);
> +    int ret;
> +
> +    ret = _parse_loglvl(s, &xenlog_lower_thresh, &xenlog_upper_thresh,
> +                        xenlog_val);
> +    custom_runtime_set_var(param_2_parfs(parse_loglvl), xenlog_val);

Mixing function and variable name is pretty confusing. It took me 
sometimes to go through the macro maze to figure out what's happening.

It might be worth thinking to document more the custom_runtime_* interface.

Cheers,

-- 
Julien Grall

Re: [PATCH for-4.14] xen/hypfs: fix loglvl parameter setting
Posted by Jürgen Groß 3 years, 9 months ago
On 10.06.20 19:55, Julien Grall wrote:
> Hi Juergen,
> 
> On 09/06/2020 16:45, Juergen Gross wrote:
>> Writing the runtime parameters loglvl or guest_loglvl omits setting the
>> new length of the resulting parameter value.
>>
>> Reported-by: George Dunlap <george.dunlap@citrix.com>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> Reviewed-by: Julien Grall <jgrall@amazon.com>
> 
> Although one unrelated comment below.
> 
>> ---
>>   xen/drivers/char/console.c | 19 +++++++++++++++----
>>   1 file changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
>> index 56e24821b2..861ad53a8f 100644
>> --- a/xen/drivers/char/console.c
>> +++ b/xen/drivers/char/console.c
>> @@ -241,14 +241,25 @@ static int _parse_loglvl(const char *s, int 
>> *lower, int *upper, char *val)
>>   static int parse_loglvl(const char *s)
>>   {
>> -    return _parse_loglvl(s, &xenlog_lower_thresh, &xenlog_upper_thresh,
>> -                         xenlog_val);
>> +    int ret;
>> +
>> +    ret = _parse_loglvl(s, &xenlog_lower_thresh, &xenlog_upper_thresh,
>> +                        xenlog_val);
>> +    custom_runtime_set_var(param_2_parfs(parse_loglvl), xenlog_val);
> 
> Mixing function and variable name is pretty confusing. It took me 
> sometimes to go through the macro maze to figure out what's happening.
> 
> It might be worth thinking to document more the custom_runtime_* interface.

I have already some streamlining ideas for 4.15.


Juergen


Re: [PATCH for-4.14] xen/hypfs: fix loglvl parameter setting
Posted by Julien Grall 3 years, 9 months ago
On Wed, 10 Jun 2020 at 19:49, Jürgen Groß <jgross@suse.com> wrote:
>
> On 10.06.20 19:55, Julien Grall wrote:
> > Hi Juergen,
> >
> > On 09/06/2020 16:45, Juergen Gross wrote:
> >> Writing the runtime parameters loglvl or guest_loglvl omits setting the
> >> new length of the resulting parameter value.
> >>
> >> Reported-by: George Dunlap <george.dunlap@citrix.com>
> >> Signed-off-by: Juergen Gross <jgross@suse.com>
> >
> > Reviewed-by: Julien Grall <jgrall@amazon.com>
> >
> > Although one unrelated comment below.
> >
> >> ---
> >>   xen/drivers/char/console.c | 19 +++++++++++++++----
> >>   1 file changed, 15 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> >> index 56e24821b2..861ad53a8f 100644
> >> --- a/xen/drivers/char/console.c
> >> +++ b/xen/drivers/char/console.c
> >> @@ -241,14 +241,25 @@ static int _parse_loglvl(const char *s, int
> >> *lower, int *upper, char *val)
> >>   static int parse_loglvl(const char *s)
> >>   {
> >> -    return _parse_loglvl(s, &xenlog_lower_thresh, &xenlog_upper_thresh,
> >> -                         xenlog_val);
> >> +    int ret;
> >> +
> >> +    ret = _parse_loglvl(s, &xenlog_lower_thresh, &xenlog_upper_thresh,
> >> +                        xenlog_val);
> >> +    custom_runtime_set_var(param_2_parfs(parse_loglvl), xenlog_val);
> >
> > Mixing function and variable name is pretty confusing. It took me
> > sometimes to go through the macro maze to figure out what's happening.
> >
> > It might be worth thinking to document more the custom_runtime_* interface.
>
> I have already some streamlining ideas for 4.15.

Cool! I will commit it tomorrow morning.

Cheers,

Re: [PATCH for-4.14] xen/hypfs: fix loglvl parameter setting
Posted by Julien Grall 3 years, 9 months ago

On 10/06/2020 22:47, Julien Grall wrote:
> On Wed, 10 Jun 2020 at 19:49, Jürgen Groß <jgross@suse.com> wrote:
>>
>> On 10.06.20 19:55, Julien Grall wrote:
>>> Hi Juergen,
>>>
>>> On 09/06/2020 16:45, Juergen Gross wrote:
>>>> Writing the runtime parameters loglvl or guest_loglvl omits setting the
>>>> new length of the resulting parameter value.
>>>>
>>>> Reported-by: George Dunlap <george.dunlap@citrix.com>
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>
>>> Reviewed-by: Julien Grall <jgrall@amazon.com>
>>>
>>> Although one unrelated comment below.
>>>
>>>> ---
>>>>    xen/drivers/char/console.c | 19 +++++++++++++++----
>>>>    1 file changed, 15 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
>>>> index 56e24821b2..861ad53a8f 100644
>>>> --- a/xen/drivers/char/console.c
>>>> +++ b/xen/drivers/char/console.c
>>>> @@ -241,14 +241,25 @@ static int _parse_loglvl(const char *s, int
>>>> *lower, int *upper, char *val)
>>>>    static int parse_loglvl(const char *s)
>>>>    {
>>>> -    return _parse_loglvl(s, &xenlog_lower_thresh, &xenlog_upper_thresh,
>>>> -                         xenlog_val);
>>>> +    int ret;
>>>> +
>>>> +    ret = _parse_loglvl(s, &xenlog_lower_thresh, &xenlog_upper_thresh,
>>>> +                        xenlog_val);
>>>> +    custom_runtime_set_var(param_2_parfs(parse_loglvl), xenlog_val);
>>>
>>> Mixing function and variable name is pretty confusing. It took me
>>> sometimes to go through the macro maze to figure out what's happening.
>>>
>>> It might be worth thinking to document more the custom_runtime_* interface.
>>
>> I have already some streamlining ideas for 4.15.
> 
> Cool! I will commit it tomorrow morning.

Actually I am missing a Released-acked-by from Paul on this patch.

Cheers,

-- 
Julien Grall

RE: [PATCH for-4.14] xen/hypfs: fix loglvl parameter setting
Posted by Paul Durrant 3 years, 9 months ago
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: 11 June 2020 10:20
> To: Julien Grall <julien.grall.oss@gmail.com>; Jürgen Groß <jgross@suse.com>; Paul Durrant
> <paul@xen.org>
> Cc: xen-devel <xen-devel@lists.xenproject.org>; Andrew Cooper <andrew.cooper3@citrix.com>; George
> Dunlap <george.dunlap@citrix.com>; Ian Jackson <ian.jackson@eu.citrix.com>; Jan Beulich
> <jbeulich@suse.com>; Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wl@xen.org>
> Subject: Re: [PATCH for-4.14] xen/hypfs: fix loglvl parameter setting
> 
> 
> 
> On 10/06/2020 22:47, Julien Grall wrote:
> > On Wed, 10 Jun 2020 at 19:49, Jürgen Groß <jgross@suse.com> wrote:
> >>
> >> On 10.06.20 19:55, Julien Grall wrote:
> >>> Hi Juergen,
> >>>
> >>> On 09/06/2020 16:45, Juergen Gross wrote:
> >>>> Writing the runtime parameters loglvl or guest_loglvl omits setting the
> >>>> new length of the resulting parameter value.
> >>>>
> >>>> Reported-by: George Dunlap <george.dunlap@citrix.com>
> >>>> Signed-off-by: Juergen Gross <jgross@suse.com>
> >>>
> >>> Reviewed-by: Julien Grall <jgrall@amazon.com>
> >>>
> >>> Although one unrelated comment below.
> >>>
> >>>> ---
> >>>>    xen/drivers/char/console.c | 19 +++++++++++++++----
> >>>>    1 file changed, 15 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> >>>> index 56e24821b2..861ad53a8f 100644
> >>>> --- a/xen/drivers/char/console.c
> >>>> +++ b/xen/drivers/char/console.c
> >>>> @@ -241,14 +241,25 @@ static int _parse_loglvl(const char *s, int
> >>>> *lower, int *upper, char *val)
> >>>>    static int parse_loglvl(const char *s)
> >>>>    {
> >>>> -    return _parse_loglvl(s, &xenlog_lower_thresh, &xenlog_upper_thresh,
> >>>> -                         xenlog_val);
> >>>> +    int ret;
> >>>> +
> >>>> +    ret = _parse_loglvl(s, &xenlog_lower_thresh, &xenlog_upper_thresh,
> >>>> +                        xenlog_val);
> >>>> +    custom_runtime_set_var(param_2_parfs(parse_loglvl), xenlog_val);
> >>>
> >>> Mixing function and variable name is pretty confusing. It took me
> >>> sometimes to go through the macro maze to figure out what's happening.
> >>>
> >>> It might be worth thinking to document more the custom_runtime_* interface.
> >>
> >> I have already some streamlining ideas for 4.15.
> >
> > Cool! I will commit it tomorrow morning.
> 
> Actually I am missing a Released-acked-by from Paul on this patch.
> 

Release-acked-by: Paul Durrant <paul@xen.org>

> Cheers,
> 
> --
> Julien Grall