Patchwork [1/3] s390: Fix empty kernel command line

login
register
mail settings
Submitter Jens Freimann
Date Dec. 7, 2012, 1:55 p.m.
Message ID <1354888531-47836-2-git-send-email-jfrei@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/204490/
State New
Headers show

Comments

Jens Freimann - Dec. 7, 2012, 1:55 p.m.
From: Christian Borntraeger <borntraeger@de.ibm.com>

Since commit 967c0da73a7b0da186baba6632301d83644a570c
    vl.c: Avoid segfault when started with no arguments

the user can specify a kernel without a command line. Lets not
overwrite the default command line with \0 in that case.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
---
 hw/s390-virtio.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Alexander Graf - Dec. 11, 2012, 10:34 a.m.
On 07.12.2012, at 14:55, Jens Freimann wrote:

> From: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> Since commit 967c0da73a7b0da186baba6632301d83644a570c
>    vl.c: Avoid segfault when started with no arguments
> 
> the user can specify a kernel without a command line. Lets not
> overwrite the default command line with \0 in that case.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
> ---
> hw/s390-virtio.c |    2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
> index ca1bb09..d77871a 100644
> --- a/hw/s390-virtio.c
> +++ b/hw/s390-virtio.c
> @@ -290,7 +290,7 @@ static void s390_init(QEMUMachineInitArgs *args)
>         stq_p(rom_ptr(INITRD_PARM_SIZE), initrd_size);
>     }
> 
> -    if (rom_ptr(KERN_PARM_AREA)) {
> +    if (rom_ptr(KERN_PARM_AREA) && strlen(kernel_cmdline)) {

why strlen()? If no -append option was passed, kernel_cmdline should be NULL. If -append "" was passed, the user wants to command line to be overwritten with "\0".


Alex
Christian Borntraeger - Dec. 11, 2012, 11:15 a.m.
On 11/12/12 11:34, Alexander Graf wrote:
> 
> On 07.12.2012, at 14:55, Jens Freimann wrote:
> 
>> From: Christian Borntraeger <borntraeger@de.ibm.com>
>>
>> Since commit 967c0da73a7b0da186baba6632301d83644a570c
>>    vl.c: Avoid segfault when started with no arguments
>>
>> the user can specify a kernel without a command line. Lets not
>> overwrite the default command line with \0 in that case.
>>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
>> ---
>> hw/s390-virtio.c |    2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
>> index ca1bb09..d77871a 100644
>> --- a/hw/s390-virtio.c
>> +++ b/hw/s390-virtio.c
>> @@ -290,7 +290,7 @@ static void s390_init(QEMUMachineInitArgs *args)
>>         stq_p(rom_ptr(INITRD_PARM_SIZE), initrd_size);
>>     }
>>
>> -    if (rom_ptr(KERN_PARM_AREA)) {
>> +    if (rom_ptr(KERN_PARM_AREA) && strlen(kernel_cmdline)) {
> 
> why strlen()? If no -append option was passed, kernel_cmdline should be NULL. If -append "" was passed, the user wants to command line to be overwritten with "\0".

Nope. kernel_cmdline is always a valid pointer.

vl.c:

[..]
    if (!kernel_cmdline) {
        kernel_cmdline = "";
    }

[..]
Alexander Graf - Dec. 11, 2012, 11:19 a.m.
On 11.12.2012, at 12:15, Christian Borntraeger wrote:

> On 11/12/12 11:34, Alexander Graf wrote:
>> 
>> On 07.12.2012, at 14:55, Jens Freimann wrote:
>> 
>>> From: Christian Borntraeger <borntraeger@de.ibm.com>
>>> 
>>> Since commit 967c0da73a7b0da186baba6632301d83644a570c
>>>   vl.c: Avoid segfault when started with no arguments
>>> 
>>> the user can specify a kernel without a command line. Lets not
>>> overwrite the default command line with \0 in that case.
>>> 
>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
>>> ---
>>> hw/s390-virtio.c |    2 +-
>>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>> 
>>> diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
>>> index ca1bb09..d77871a 100644
>>> --- a/hw/s390-virtio.c
>>> +++ b/hw/s390-virtio.c
>>> @@ -290,7 +290,7 @@ static void s390_init(QEMUMachineInitArgs *args)
>>>        stq_p(rom_ptr(INITRD_PARM_SIZE), initrd_size);
>>>    }
>>> 
>>> -    if (rom_ptr(KERN_PARM_AREA)) {
>>> +    if (rom_ptr(KERN_PARM_AREA) && strlen(kernel_cmdline)) {
>> 
>> why strlen()? If no -append option was passed, kernel_cmdline should be NULL. If -append "" was passed, the user wants to command line to be overwritten with "\0".
> 
> Nope. kernel_cmdline is always a valid pointer.
> 
> vl.c:
> 
> [..]
>    if (!kernel_cmdline) {
>        kernel_cmdline = "";
>    }

Then that's on purpose. Either we change the default for everyone or not at all. But checking for "" only in the s390 machine sounds off.


Alex
Christian Borntraeger - Dec. 11, 2012, 11:26 a.m.
On 11/12/12 12:19, Alexander Graf wrote:
> 
> On 11.12.2012, at 12:15, Christian Borntraeger wrote:
> 
>> On 11/12/12 11:34, Alexander Graf wrote:
>>>
>>> On 07.12.2012, at 14:55, Jens Freimann wrote:
>>>
>>>> From: Christian Borntraeger <borntraeger@de.ibm.com>
>>>>
>>>> Since commit 967c0da73a7b0da186baba6632301d83644a570c
>>>>   vl.c: Avoid segfault when started with no arguments
>>>>
>>>> the user can specify a kernel without a command line. Lets not
>>>> overwrite the default command line with \0 in that case.
>>>>
>>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
>>>> ---
>>>> hw/s390-virtio.c |    2 +-
>>>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
>>>> index ca1bb09..d77871a 100644
>>>> --- a/hw/s390-virtio.c
>>>> +++ b/hw/s390-virtio.c
>>>> @@ -290,7 +290,7 @@ static void s390_init(QEMUMachineInitArgs *args)
>>>>        stq_p(rom_ptr(INITRD_PARM_SIZE), initrd_size);
>>>>    }
>>>>
>>>> -    if (rom_ptr(KERN_PARM_AREA)) {
>>>> +    if (rom_ptr(KERN_PARM_AREA) && strlen(kernel_cmdline)) {
>>>
>>> why strlen()? If no -append option was passed, kernel_cmdline should be NULL. If -append "" was passed, the user wants to command line to be overwritten with "\0".
>>
>> Nope. kernel_cmdline is always a valid pointer.
>>
>> vl.c:
>>
>> [..]
>>    if (!kernel_cmdline) {
>>        kernel_cmdline = "";
>>    }
> 
> Then that's on purpose. Either we change the default for everyone or not at all. But checking for "" only in the s390 machine sounds off.

Ok, makes sense.

Patch

diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
index ca1bb09..d77871a 100644
--- a/hw/s390-virtio.c
+++ b/hw/s390-virtio.c
@@ -290,7 +290,7 @@  static void s390_init(QEMUMachineInitArgs *args)
         stq_p(rom_ptr(INITRD_PARM_SIZE), initrd_size);
     }
 
-    if (rom_ptr(KERN_PARM_AREA)) {
+    if (rom_ptr(KERN_PARM_AREA) && strlen(kernel_cmdline)) {
         /* we have to overwrite values in the kernel image, which are "rom" */
         memcpy(rom_ptr(KERN_PARM_AREA), kernel_cmdline,
                strlen(kernel_cmdline) + 1);