[PATCH v2] panic: setup panic_timeout early

[PATCH v2] panic: setup panic_timeout early

[Felipe Contreras]

Otherwise we might not reboot when the user needs it the most (early
on).

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---

This time using kstrtol() instead of get_option().

Interdiff:

diff --git a/kernel/panic.c b/kernel/panic.c
index 46e37ff..d865263 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -470,12 +470,14 @@ EXPORT_SYMBOL(__stack_chk_fail);
 
 core_param(pause_on_oops, pause_on_oops, int, 0644);
 
-static int __init set_panic_timeout(char *str)
+static int __init set_panic_timeout(char *val)
 {
-	int timeout;
+	long timeout;
+	int ret;
 
-	if (!get_option(&str, &timeout))
-		return -EINVAL;
+	ret = kstrtol(val, 0, &timeout);
+	if (ret < 0)
+		return ret;
 
 	panic_timeout = timeout;
 	return 0;

 kernel/panic.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index b6c482c..d865263 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -468,9 +468,23 @@ EXPORT_SYMBOL(__stack_chk_fail);
 
 #endif
 
-core_param(panic, panic_timeout, int, 0644);
 core_param(pause_on_oops, pause_on_oops, int, 0644);
 
+static int __init set_panic_timeout(char *val)
+{
+	long timeout;
+	int ret;
+
+	ret = kstrtol(val, 0, &timeout);
+	if (ret < 0)
+		return ret;
+
+	panic_timeout = timeout;
+	return 0;
+}
+
+early_param("panic_timeout", set_panic_timeout);
+
 static int __init oops_setup(char *s)
 {
 	if (!s)
-- 
1.8.4.2+fc1

Re: [PATCH v2] panic: setup panic_timeout early

[Ingo Molnar]

* Felipe Contreras <felipe.contreras@gmail.com> wrote:

> Otherwise we might not reboot when the user needs it the most (early
> on).
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
> 
[...]
>
> diff --git a/kernel/panic.c b/kernel/panic.c
> index b6c482c..d865263 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -468,9 +468,23 @@ EXPORT_SYMBOL(__stack_chk_fail);
>  
>  #endif
>  
> -core_param(panic, panic_timeout, int, 0644);
>  core_param(pause_on_oops, pause_on_oops, int, 0644);
>  
> +static int __init set_panic_timeout(char *val)
> +{
> +	long timeout;
> +	int ret;
> +
> +	ret = kstrtol(val, 0, &timeout);
> +	if (ret < 0)
> +		return ret;
> +
> +	panic_timeout = timeout;
> +	return 0;
> +}

I think the type of the 'timeout' local variable should match the type of 
'panic_timeout' (which is 'int', not 'long').

Thanks,

	Ingo

Re: [PATCH v2] panic: setup panic_timeout early

[Felipe Contreras]

On Tue, Nov 12, 2013 at 6:03 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Felipe Contreras <felipe.contreras@gmail.com> wrote:
>
>> Otherwise we might not reboot when the user needs it the most (early
>> on).
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>
> [...]
>>
>> diff --git a/kernel/panic.c b/kernel/panic.c
>> index b6c482c..d865263 100644
>> --- a/kernel/panic.c
>> +++ b/kernel/panic.c
>> @@ -468,9 +468,23 @@ EXPORT_SYMBOL(__stack_chk_fail);
>>
>>  #endif
>>
>> -core_param(panic, panic_timeout, int, 0644);
>>  core_param(pause_on_oops, pause_on_oops, int, 0644);
>>
>> +static int __init set_panic_timeout(char *val)
>> +{
>> +     long timeout;
>> +     int ret;
>> +
>> +     ret = kstrtol(val, 0, &timeout);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     panic_timeout = timeout;
>> +     return 0;
>> +}
>
> I think the type of the 'timeout' local variable should match the type of
> 'panic_timeout' (which is 'int', not 'long').

So you would rather have this?

  kstrtol(val, 0, (long *)&timeout);

Couldn't that potentially write the value beyond the memory allocated
to 'timeout'?

-- 
Felipe Contreras

Re: [PATCH v2] panic: setup panic_timeout early

[Levente Kurusa]

2013-11-14 12:16 keltezéssel, Felipe Contreras írta:
> On Tue, Nov 12, 2013 at 6:03 PM, Ingo Molnar <mingo@kernel.org> wrote:
>>
>> * Felipe Contreras <felipe.contreras@gmail.com> wrote:
>>
>>> Otherwise we might not reboot when the user needs it the most (early
>>> on).
>>>
>>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>>> ---
>>>
>> [...]
>>>
>>> diff --git a/kernel/panic.c b/kernel/panic.c
>>> index b6c482c..d865263 100644
>>> --- a/kernel/panic.c
>>> +++ b/kernel/panic.c
>>> @@ -468,9 +468,23 @@ EXPORT_SYMBOL(__stack_chk_fail);
>>>
>>>  #endif
>>>
>>> -core_param(panic, panic_timeout, int, 0644);
>>>  core_param(pause_on_oops, pause_on_oops, int, 0644);
>>>
>>> +static int __init set_panic_timeout(char *val)
>>> +{
>>> +     long timeout;
>>> +     int ret;
>>> +
>>> +     ret = kstrtol(val, 0, &timeout);
>>> +     if (ret < 0)
>>> +             return ret;
>>> +
>>> +     panic_timeout = timeout;
>>> +     return 0;
>>> +}
>>
>> I think the type of the 'timeout' local variable should match the type of
>> 'panic_timeout' (which is 'int', not 'long').
> 
> So you would rather have this?
> 
>   kstrtol(val, 0, (long *)&timeout);
> 
> Couldn't that potentially write the value beyond the memory allocated
> to 'timeout'?
> 

No, 'panic_timeout' is a variable of type 'int'.
Your 'long timeout;' line is wrong and should say 'int timeout;'


-- 
Regards,
Levente Kurusa

Re: [PATCH v2] panic: setup panic_timeout early

[Felipe Contreras]

Hi,

Sending again since the previous one was rejected by the server.

On Thu, Nov 14, 2013 at 3:25 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> Levente Kurusa wrote:
>> 2013-11-14 12:16 keltezéssel, Felipe Contreras írta:
>> > On Tue, Nov 12, 2013 at 6:03 PM, Ingo Molnar <mingo@kernel.org> wrote:
>> >>
>> >> * Felipe Contreras <felipe.contreras@gmail.com> wrote:
>> >>
>> >>> Otherwise we might not reboot when the user needs it the most (early
>> >>> on).
>> >>>
>> >>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> >>> ---
>> >>>
>> >> [...]
>> >>>
>> >>> diff --git a/kernel/panic.c b/kernel/panic.c
>> >>> index b6c482c..d865263 100644
>> >>> --- a/kernel/panic.c
>> >>> +++ b/kernel/panic.c
>> >>> @@ -468,9 +468,23 @@ EXPORT_SYMBOL(__stack_chk_fail);
>> >>>
>> >>>  #endif
>> >>>
>> >>> -core_param(panic, panic_timeout, int, 0644);
>> >>>  core_param(pause_on_oops, pause_on_oops, int, 0644);
>> >>>
>> >>> +static int __init set_panic_timeout(char *val)
>> >>> +{
>> >>> +     long timeout;
>> >>> +     int ret;
>> >>> +
>> >>> +     ret = kstrtol(val, 0, &timeout);
>> >>> +     if (ret < 0)
>> >>> +             return ret;
>> >>> +
>> >>> +     panic_timeout = timeout;
>> >>> +     return 0;
>> >>> +}
>> >>
>> >> I think the type of the 'timeout' local variable should match the type of
>> >> 'panic_timeout' (which is 'int', not 'long').
>> >
>> > So you would rather have this?
>> >
>> >   kstrtol(val, 0, (long *)&timeout);
>> >
>> > Couldn't that potentially write the value beyond the memory allocated
>> > to 'timeout'?
>> >
>>
>> No, 'panic_timeout' is a variable of type 'int'.
>> Your 'long timeout;' line is wrong and should say 'int timeout;'
>
> Oh really? Something like this?
>
>   --- a/kernel/panic.c
>   +++ b/kernel/panic.c
>   @@ -472,7 +472,7 @@ core_param(pause_on_oops, pause_on_oops, int, 0644);
>
>    static int __init set_panic_timeout(char *val)
>    {
>   -       long timeout;
>   +       int timeout;
>           int ret;
>
>           ret = kstrtol(val, 0, &timeout);
>
> And then what happens?
>
>   kernel/panic.c: In function ‘set_panic_timeout’:
>   kernel/panic.c:478:2: warning: passing argument 3 of ‘kstrtol’ from incompatible pointer type [enabled by default]
>     ret = kstrtol(val, 0, &timeout);
>     ^
>   In file included from include/linux/debug_locks.h:4:0,
>                    from kernel/panic.c:11:
>   include/linux/kernel.h:268:95: note: expected ‘long int *’ but argument is of type ‘int *’
>    static inline int __must_check kstrtol(const char *s, unsigned int base, long *res)
>
> To fix that warning you need this:
>                                                                                                  ^
>   --- a/kernel/panic.c
>   +++ b/kernel/panic.c
>   @@ -472,10 +472,10 @@ core_param(pause_on_oops, pause_on_oops, int, 0644);
>
>    static int __init set_panic_timeout(char *val)
>    {
>   -       long timeout;
>   +       int timeout;
>           int ret;
>
>   -       ret = kstrtol(val, 0, &timeout);
>   +       ret = kstrtol(val, 0, (long *)&timeout);
>           if (ret < 0)
>                   return ret;
>
>
> Which is the only logical conclusion I arrived to. What else do you suggest to
> fix the problem that kstrtol() expects a long? And since this fix is not what
> we want because we would be writing to the wrong memory, we don't want 'timeout'
> to be int.
>
> Therefore 'timeout' should be a long. How is that not clear?
>
> You can even see that it's already done this way for parameters:
>
>   STANDARD_PARAM_DEF(int, int, "%i", long, kstrtol);
>
>   #define STANDARD_PARAM_DEF(name, type, format, tmptype, strtolfn)             \
>           int param_set_##name(const char *val, const struct kernel_param *kp) \
>           {                                                             \
>                   tmptype l;                                            \
>                   int ret;                                              \
>                                                                           \
>                   ret = strtolfn(val, 0, &l);                           \
>                   if (ret < 0 || ((type)l != l))                                \
>                           return ret < 0 ? ret : -EINVAL;                       \
>                   *((type *)kp->arg) = l;                                       \
>                   return 0;                                             \
>           }                                                             \
>
> So yes, we obviously want the temporary variable 'timeout' to be a long, even
> though the final destination is an int.

-- 
Felipe Contreras

Re: [PATCH v2] panic: setup panic_timeout early

[Levente Kurusa]

2013-11-14 23:32 keltezéssel, Felipe Contreras írta:
> Hi,
> 
> Sending again since the previous one was rejected by the server.
> 
> On Thu, Nov 14, 2013 at 3:25 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> Levente Kurusa wrote:
>>> 2013-11-14 12:16 keltezéssel, Felipe Contreras írta:
>>>> On Tue, Nov 12, 2013 at 6:03 PM, Ingo Molnar <mingo@kernel.org> wrote:
>>>>>
>>>>> * Felipe Contreras <felipe.contreras@gmail.com> wrote:
>>>>>
>>>>>> Otherwise we might not reboot when the user needs it the most (early
>>>>>> on).
>>>>>>
>>>>>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>>>>>> ---
>>>>>>
>>>>> [...]
>>>>>>
>>>>>> diff --git a/kernel/panic.c b/kernel/panic.c
>>>>>> index b6c482c..d865263 100644
>>>>>> --- a/kernel/panic.c
>>>>>> +++ b/kernel/panic.c
>>>>>> @@ -468,9 +468,23 @@ EXPORT_SYMBOL(__stack_chk_fail);
>>>>>>
>>>>>>  #endif
>>>>>>
>>>>>> -core_param(panic, panic_timeout, int, 0644);
>>>>>>  core_param(pause_on_oops, pause_on_oops, int, 0644);
>>>>>>
>>>>>> +static int __init set_panic_timeout(char *val)
>>>>>> +{
>>>>>> +     long timeout;
>>>>>> +     int ret;
>>>>>> +
>>>>>> +     ret = kstrtol(val, 0, &timeout);
>>>>>> +     if (ret < 0)
>>>>>> +             return ret;
>>>>>> +
>>>>>> +     panic_timeout = timeout;
>>>>>> +     return 0;
>>>>>> +}
>>>>>
>>>>> I think the type of the 'timeout' local variable should match the type of
>>>>> 'panic_timeout' (which is 'int', not 'long').
>>>>
>>>> So you would rather have this?
>>>>
>>>>   kstrtol(val, 0, (long *)&timeout);
>>>>
>>>> Couldn't that potentially write the value beyond the memory allocated
>>>> to 'timeout'?
>>>>
>>>
>>> No, 'panic_timeout' is a variable of type 'int'.
>>> Your 'long timeout;' line is wrong and should say 'int timeout;'
>>
>> Oh really? Something like this?
>>
>>   --- a/kernel/panic.c
>>   +++ b/kernel/panic.c
>>   @@ -472,7 +472,7 @@ core_param(pause_on_oops, pause_on_oops, int, 0644);
>>
>>    static int __init set_panic_timeout(char *val)
>>    {
>>   -       long timeout;
>>   +       int timeout;
>>           int ret;
>>
>>           ret = kstrtol(val, 0, &timeout);
>>
>> And then what happens?
>>
>>   kernel/panic.c: In function ‘set_panic_timeout’:
>>   kernel/panic.c:478:2: warning: passing argument 3 of ‘kstrtol’ from incompatible pointer type [enabled by default]
>>     ret = kstrtol(val, 0, &timeout);
>>     ^
>>   In file included from include/linux/debug_locks.h:4:0,
>>                    from kernel/panic.c:11:
>>   include/linux/kernel.h:268:95: note: expected ‘long int *’ but argument is of type ‘int *’
>>    static inline int __must_check kstrtol(const char *s, unsigned int base, long *res)
>>
>> To fix that warning you need this:
>>                                                                                                  ^
>>   --- a/kernel/panic.c
>>   +++ b/kernel/panic.c
>>   @@ -472,10 +472,10 @@ core_param(pause_on_oops, pause_on_oops, int, 0644);
>>
>>    static int __init set_panic_timeout(char *val)
>>    {
>>   -       long timeout;
>>   +       int timeout;
>>           int ret;
>>
>>   -       ret = kstrtol(val, 0, &timeout);
>>   +       ret = kstrtol(val, 0, (long *)&timeout);
>>           if (ret < 0)
>>                   return ret;
>>
>>
>> Which is the only logical conclusion I arrived to. What else do you suggest to
>> fix the problem that kstrtol() expects a long? And since this fix is not what
>> we want because we would be writing to the wrong memory, we don't want 'timeout'
>> to be int.
>>
>> Therefore 'timeout' should be a long. How is that not clear?
>>
>> You can even see that it's already done this way for parameters:
>>
>>   STANDARD_PARAM_DEF(int, int, "%i", long, kstrtol);
>>
>>   #define STANDARD_PARAM_DEF(name, type, format, tmptype, strtolfn)             \
>>           int param_set_##name(const char *val, const struct kernel_param *kp) \
>>           {                                                             \
>>                   tmptype l;                                            \
>>                   int ret;                                              \
>>                                                                           \
>>                   ret = strtolfn(val, 0, &l);                           \
>>                   if (ret < 0 || ((type)l != l))                                \
>>                           return ret < 0 ? ret : -EINVAL;                       \
>>                   *((type *)kp->arg) = l;                                       \
>>                   return 0;                                             \
>>           }                                                             \
>>
>> So yes, we obviously want the temporary variable 'timeout' to be a long, even
>> though the final destination is an int.
> 

Hi,

No, you don't want timeout to be a long, and instead of kstrtol, you should use
kstrtoint(char *s, int base, int *res)
which is defined in lib/ktstrtox.c

Also, a bit of advice: Calm down. If you continue to act childish nobody will take
you seriously. Before acting like you are the king here, please at least take the time
to research other options.

-- 
Regards,
Levente Kurusa

Re: [PATCH v2] panic: setup panic_timeout early

[Felipe Contreras]

On Fri, Nov 15, 2013 at 7:12 AM, Levente Kurusa <levex@linux.com> wrote:

> No, you don't want timeout to be a long, and instead of kstrtol, you should use
> kstrtoint(char *s, int base, int *res)
> which is defined in lib/ktstrtox.c

And if you look into kstrtoint() you would see that it's doing
*exactly* the same I'm doing; using a temporary bigger integer, same
as STANDARD_PARAM_DEF.

> Also, a bit of advice: Calm down. If you continue to act childish nobody will take
> you seriously. Before acting like you are the king here, please at least take the time
> to research other options.

I am irrelevant, the code is what matters, and panic_timeout should be
set early on, regardless of who is sending the patch.

Moreover, if you are going to claim that I didn't research other
options, then the guy that did this:

  STANDARD_PARAM_DEF(int, int, "%i", long, kstrtol);

Didn't research other options either, because it should be:

  STANDARD_PARAM_DEF(int, int, "%i", int, kstrtoint);

Presumably it doesn't matter much, but it seems it does matter when
*I* am the one sending the patch.

And what exactly is childish about asking this question?

> So you would rather have this?
>
>   kstrtol(val, 0, (long *)&timeout);

The answer should be: no, use kstrtoint(), but that wasn't your
answer, was it? Your answer basically repeated what Ingo said, so I
had to go step by step to the conclusion of 'kstrtol(val, 0, (long
*)&timeout);'. And then I asked you a question: "What else do you
suggest to fix the problem that kstrtol() expects a long?" *Now* your
answer is useful. Why is that childish?

Now, in this process a few things are clear to me:

1) loglevel should use kstrtoint() as well, not get_option
2) get_option() should use kstrtoint(); it's using simple_strtol() and
it's not even checking for overflows
3) It should be STANDARD_PARAM_DEF(int, int, "%i", int, kstrtoint);

Don't you think it's little bit of a stretch to say that *I* didn't do
my research, when in fact I did, and none of the parameters code is
using kstrtoint() as it should, even Ingo himself told me I should use
simple_strtol(), which is actually deprecated.

My patch is simply doing what similar code is already doing. Why don't
you take a step back and accept the possibility that a) I did do my
research, and b) I wasn't being childish in asking how to make kstrtol
work (given that Ingo suggested to use simple_strtol()). I think you
should take your own advice and calm down, maybe even take back what
you said. Maybe I was combative, and maybe I made the wrong
assumptions, but at least I didn't throw insults, nor called anybody
names like "childish".

Anyway, I will update the patch to sue kstrtoint(), and I would gladly
fix the existing code for issues 1) 2) and 3) *if* you or anybody
agrees they should be using kstrtoint() as well, I'm not in the mood
of going through Ingo's review process again for something so trivial.

Cheers.

-- 
Felipe Contreras

Re: [PATCH v2] panic: setup panic_timeout early

[Linus Torvalds]

On Fri, Nov 15, 2013 at 11:33 AM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> I am irrelevant, the code is what matter

Not so. You are relevant in the sense that you need to communicate why
people should take patches from you.

I know people who just put you in their kill-files because they can't
bother with the endless argumentative flames you generate. At which
point all the code you produce is kind of irrelevant, because people
won't see it.

Seriously, felipe. You pissed off Junio, who is *hard* to annoy. Just
learn from your mistakes, instead of repeating them over and over.

It's not just code. You need to also explain *why* people should apply
it, and stop the f*cking idiotic arguing every time somebody comments
about your patches.

Because it literally seems to be *EVERY* single time.

          Linus

Re: [PATCH v2] panic: setup panic_timeout early

[Felipe Contreras]

On Fri, Nov 15, 2013 at 1:55 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:

> It's not just code. You need to also explain *why* people should apply
> it, and stop the f*cking idiotic arguing every time somebody comments
> about your patches.

Doesn't this explain it?

------------------------------------------------------------------------------
panic: setup panic_timeout early

Otherwise we might not reboot when the user needs it the most (early
on).
------------------------------------------------------------------------------

I haven't seen a single complaint about this commit message, so I
don't see what is your point.

You didn't address what I said at all. If the code is technically
correct *and* it is clear there's a reason why the patch should be
applied, who sent the patch should be irrelevant, because even if that
person is problematic, and there's something lacking in the patch,
somebody else can take it from there and fix the remaining issues,
because if there's a reason the patch should be applied, it should be
applied.

In this particular case it is clear we want panic_timeout to work
early on, therefore a patch should be applied to achieve that, and you
might as well simply apply the patch I sent, even though *I* sent it,
because it's technically correct, and the need is explained. Why
wouldn't you?

-- 
Felipe Contreras

Re: [PATCH v2] panic: setup panic_timeout early

[Linus Torvalds]

On Fri, Nov 15, 2013 at 12:10 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> I haven't seen a single complaint about this commit message, so I
> don't see what is your point.

My point is that I have sixteen pointless messages in my mbox, half of
which are due to just your argumentative nature.

In other words, my point is that you are wasting my - and other
peoples - time just because you *always* have to turn every single
thing into a big argument when somebody comments on a patch.

              Linus

Re: [PATCH v2] panic: setup panic_timeout early

[Felipe Contreras]

On Fri, Nov 15, 2013 at 2:15 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Nov 15, 2013 at 12:10 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>>
>> I haven't seen a single complaint about this commit message, so I
>> don't see what is your point.
>
> My point is that I have sixteen pointless messages in my mbox, half of
> which are due to just your argumentative nature.

This is clearly not the point you were making, but I won't argue why.

You don't want to argue? Then don't argue. Apply the patch. Unless you
see something wrong with the patch. Do you see something wrong with
the patch?

-- 
Felipe Contreras

Re: [PATCH v2] panic: setup panic_timeout early

[Ingo Molnar]

* Felipe Contreras <felipe.contreras@gmail.com> wrote:

> On Fri, Nov 15, 2013 at 2:15 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> > On Fri, Nov 15, 2013 at 12:10 PM, Felipe Contreras
> > <felipe.contreras@gmail.com> wrote:
> >>
> >> I haven't seen a single complaint about this commit message, so I 
> >> don't see what is your point.
> >
> > My point is that I have sixteen pointless messages in my mbox, 
> > half of which are due to just your argumentative nature.
> 
> This is clearly not the point you were making, but I won't argue 
> why.

That was exactly the point Linus was making.

Anyway, the fact that you are argumentative even with Linus gives me 
little hope that you will improve your communication patterns with 
_me_, so I'm personally done arguing with you.

> You don't want to argue? Then don't argue. Apply the patch. [...]

*Plonk*.

	Ingo

Re: [PATCH v2] panic: setup panic_timeout early

[Felipe Contreras]

On Sat, Nov 16, 2013 at 11:45 AM, Ingo Molnar <mingo@kernel.org> wrote:
> * Felipe Contreras <felipe.contreras@gmail.com> wrote:

> Anyway, the fact that you are argumentative even with Linus gives me
> little hope that you will improve your communication patterns with
> _me_, so I'm personally done arguing with you.

How am I being argumentative? I avoided all arguments.

>> You don't want to argue? Then don't argue. Apply the patch. [...]
>
> *Plonk*.

This is exactly the opposite of what is conducive to less argumentation.

If you are not going to comment on the patch, then don't say anything.

The patch is good, and the fact that both you and Linus are avoiding
any comments in the patch itself doesn't speak well for your
intentions to avoid argumentation.

So I ask you again. Do you see something wrong with the patch?

-- 
Felipe Contreras

Re: [PATCH v2] panic: setup panic_timeout early

[Levente Kurusa]

2013-11-16 19:46 keltezéssel, Felipe Contreras írta:
> On Sat, Nov 16, 2013 at 11:45 AM, Ingo Molnar <mingo@kernel.org> wrote:
>> * Felipe Contreras <felipe.contreras@gmail.com> wrote:
> 
>> Anyway, the fact that you are argumentative even with Linus gives me
>> little hope that you will improve your communication patterns with
>> _me_, so I'm personally done arguing with you.
> 
> How am I being argumentative? I avoided all arguments.
> 
>>> You don't want to argue? Then don't argue. Apply the patch. [...]
>>
>> *Plonk*.
> 
> This is exactly the opposite of what is conducive to less argumentation.
> 
> If you are not going to comment on the patch, then don't say anything.
> 
> The patch is good, and the fact that both you and Linus are avoiding
> any comments in the patch itself doesn't speak well for your
> intentions to avoid argumentation.
> 
> So I ask you again. Do you see something wrong with the patch?
> 

Yes, you.

-- 
Regards,
Levente Kurusa

Re: [PATCH v2] panic: setup panic_timeout early

[Levente Kurusa]

2013-11-16 19:46 keltezéssel, Felipe Contreras írta:
> On Sat, Nov 16, 2013 at 11:45 AM, Ingo Molnar <mingo@kernel.org> wrote:
>> * Felipe Contreras <felipe.contreras@gmail.com> wrote:
> 
>> Anyway, the fact that you are argumentative even with Linus gives me
>> little hope that you will improve your communication patterns with
>> _me_, so I'm personally done arguing with you.
> 
> How am I being argumentative? I avoided all arguments.
> 
>>> You don't want to argue? Then don't argue. Apply the patch. [...]
>>
>> *Plonk*.
> 
> This is exactly the opposite of what is conducive to less argumentation.
> 
> If you are not going to comment on the patch, then don't say anything.
> 
> The patch is good, and the fact that both you and Linus are avoiding
> any comments in the patch itself doesn't speak well for your
> intentions to avoid argumentation.
> 
> So I ask you again. Do you see something wrong with the patch?
> 

Out-of-joke, the patch is now in an 'appliable' state.

You can add my:
Reviewed-by: Levente Kurusa <levex@linux.com>

-- 
Regards,
Levente Kurusa

Re: [PATCH v2] panic: setup panic_timeout early

[Ingo Molnar]

* Felipe Contreras <felipe.contreras@gmail.com> wrote:

> On Tue, Nov 12, 2013 at 6:03 PM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> >> +static int __init set_panic_timeout(char *val)
> >> +{
> >> +     long timeout;
> >> +     int ret;
> >> +
> >> +     ret = kstrtol(val, 0, &timeout);
> >> +     if (ret < 0)
> >> +             return ret;
> >> +
> >> +     panic_timeout = timeout;
> >> +     return 0;
> >> +}
> >
> > I think the type of the 'timeout' local variable should match the type of
> > 'panic_timeout' (which is 'int', not 'long').
> 
> So you would rather have this?
> 
>   kstrtol(val, 0, (long *)&timeout);
> 
> Couldn't that potentially write the value beyond the memory 
> allocated to 'timeout'?

No, casting that to 'long *' is actively wrong, I'd use a
string -> integer conversion method that deals with ints,
not longs, such as kstrtoint().

Thanks,

	Ingo