Patchwork [7/7] vl: Tighten parsing of -machine option phandle_start

login
register
mail settings
Submitter Markus Armbruster
Date July 4, 2013, 1:09 p.m.
Message ID <1372943363-24081-8-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/256898/
State New
Headers show

Comments

Markus Armbruster - July 4, 2013, 1:09 p.m.
Make it QEMU_OPT_NUMBER, so it gets parsed by generic code, which
actually bothers to check for errors, rather than its user, which
doesn't.

Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 device_tree.c | 7 ++-----
 vl.c          | 2 +-
 2 files changed, 3 insertions(+), 6 deletions(-)
Alexander Graf - July 4, 2013, 1:31 p.m.
On 04.07.2013, at 15:09, Markus Armbruster wrote:

> Make it QEMU_OPT_NUMBER, so it gets parsed by generic code, which
> actually bothers to check for errors, rather than its user, which
> doesn't.
> 
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> device_tree.c | 7 ++-----
> vl.c          | 2 +-
> 2 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/device_tree.c b/device_tree.c
> index 0e7fe2d..10cf3d0 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -240,11 +240,8 @@ uint32_t qemu_devtree_alloc_phandle(void *fdt)
>      * which phandle id to start allocting phandles.
>      */
>     if (!phandle) {
> -        const char *phandle_start = qemu_opt_get(qemu_get_machine_opts(),
> -                                                 "phandle_start");
> -        if (phandle_start) {
> -            phandle = strtoul(phandle_start, NULL, 0);
> -        }
> +        phandle = qemu_opt_get_number(qemu_get_machine_opts(),
> +                                      "phandle_start", 0);

Zero is a valid phandle to start from. It shouldn't mean "default".


Alex

>     }
> 
>     if (!phandle) {
> diff --git a/vl.c b/vl.c
> index fb69f22..bea1a10 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -409,7 +409,7 @@ static QemuOptsList qemu_machine_opts = {
>             .help = "Dump current dtb to a file and quit",
>         }, {
>             .name = "phandle_start",
> -            .type = QEMU_OPT_STRING,
> +            .type = QEMU_OPT_NUMBER,
>             .help = "The first phandle ID we may generate dynamically",
>         }, {
>             .name = "dt_compatible",
> -- 
> 1.7.11.7
>
Markus Armbruster - July 4, 2013, 3:01 p.m.
Alexander Graf <agraf@suse.de> writes:

> On 04.07.2013, at 15:09, Markus Armbruster wrote:
>
>> Make it QEMU_OPT_NUMBER, so it gets parsed by generic code, which
>> actually bothers to check for errors, rather than its user, which
>> doesn't.
>> 
>> Cc: Alexander Graf <agraf@suse.de>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> device_tree.c | 7 ++-----
>> vl.c          | 2 +-
>> 2 files changed, 3 insertions(+), 6 deletions(-)
>> 
>> diff --git a/device_tree.c b/device_tree.c
>> index 0e7fe2d..10cf3d0 100644
>> --- a/device_tree.c
>> +++ b/device_tree.c
>> @@ -240,11 +240,8 @@ uint32_t qemu_devtree_alloc_phandle(void *fdt)
   uint32_t qemu_devtree_alloc_phandle(void *fdt)
   {
       static int phandle = 0x0;

       /*
>>      * which phandle id to start allocting phandles.
>>      */
>>     if (!phandle) {
>> -        const char *phandle_start = qemu_opt_get(qemu_get_machine_opts(),
>> -                                                 "phandle_start");
>> -        if (phandle_start) {
>> -            phandle = strtoul(phandle_start, NULL, 0);
>> -        }
>> +        phandle = qemu_opt_get_number(qemu_get_machine_opts(),
>> +                                      "phandle_start", 0);
>
> Zero is a valid phandle to start from. It shouldn't mean "default".

We get here only when phandle is zero (which it initially is).

If opts don't contain a value for "phandle_start", we set phandle to
zero, i.e. do nothing.  Exactly the same as before.

If that's wrong, it should be fixed in a separate patch.
Alexander Graf - July 4, 2013, 11:21 p.m.
On 04.07.2013, at 17:01, Markus Armbruster wrote:

> Alexander Graf <agraf@suse.de> writes:
> 
>> On 04.07.2013, at 15:09, Markus Armbruster wrote:
>> 
>>> Make it QEMU_OPT_NUMBER, so it gets parsed by generic code, which
>>> actually bothers to check for errors, rather than its user, which
>>> doesn't.
>>> 
>>> Cc: Alexander Graf <agraf@suse.de>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>> device_tree.c | 7 ++-----
>>> vl.c          | 2 +-
>>> 2 files changed, 3 insertions(+), 6 deletions(-)
>>> 
>>> diff --git a/device_tree.c b/device_tree.c
>>> index 0e7fe2d..10cf3d0 100644
>>> --- a/device_tree.c
>>> +++ b/device_tree.c
>>> @@ -240,11 +240,8 @@ uint32_t qemu_devtree_alloc_phandle(void *fdt)
>   uint32_t qemu_devtree_alloc_phandle(void *fdt)
>   {
>       static int phandle = 0x0;
> 
>       /*
>>>     * which phandle id to start allocting phandles.
>>>     */
>>>    if (!phandle) {
>>> -        const char *phandle_start = qemu_opt_get(qemu_get_machine_opts(),
>>> -                                                 "phandle_start");
>>> -        if (phandle_start) {
>>> -            phandle = strtoul(phandle_start, NULL, 0);
>>> -        }
>>> +        phandle = qemu_opt_get_number(qemu_get_machine_opts(),
>>> +                                      "phandle_start", 0);
>> 
>> Zero is a valid phandle to start from. It shouldn't mean "default".
> 
> We get here only when phandle is zero (which it initially is).
> 
> If opts don't contain a value for "phandle_start", we set phandle to
> zero, i.e. do nothing.  Exactly the same as before.

True. Sorry for the fuss.

Acked-by: Alexander Graf <agraf@suse.de>


Alex

> 
> If that's wrong, it should be fixed in a separate patch.

Patch

diff --git a/device_tree.c b/device_tree.c
index 0e7fe2d..10cf3d0 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -240,11 +240,8 @@  uint32_t qemu_devtree_alloc_phandle(void *fdt)
      * which phandle id to start allocting phandles.
      */
     if (!phandle) {
-        const char *phandle_start = qemu_opt_get(qemu_get_machine_opts(),
-                                                 "phandle_start");
-        if (phandle_start) {
-            phandle = strtoul(phandle_start, NULL, 0);
-        }
+        phandle = qemu_opt_get_number(qemu_get_machine_opts(),
+                                      "phandle_start", 0);
     }
 
     if (!phandle) {
diff --git a/vl.c b/vl.c
index fb69f22..bea1a10 100644
--- a/vl.c
+++ b/vl.c
@@ -409,7 +409,7 @@  static QemuOptsList qemu_machine_opts = {
             .help = "Dump current dtb to a file and quit",
         }, {
             .name = "phandle_start",
-            .type = QEMU_OPT_STRING,
+            .type = QEMU_OPT_NUMBER,
             .help = "The first phandle ID we may generate dynamically",
         }, {
             .name = "dt_compatible",