[Qemu-devel] [PATCH 1/2] qdev: factor out qdev_print_devinfo.

[Qemu-devel] [PATCH 1/2] qdev: factor out qdev_print_devinfo.

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/qdev.c |   19 ++++++++++++++++++-
 1 files changed, 18 insertions(+), 1 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 479eb72..6f05232 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -105,6 +105,21 @@ DeviceState *qdev_create(BusState *bus, const char *name)
     return dev;
 }
 
+static int qdev_print_devinfo(DeviceInfo *info, char *dest, int len)
+{
+    int pos = 0;
+
+    pos += snprintf(dest+pos, len-pos, "name \"%s\", bus %s",
+                    info->name, info->bus_info->name);
+    if (info->alias)
+        pos += snprintf(dest+pos, len-pos, ", alias \"%s\"", info->alias);
+    if (info->desc)
+        pos += snprintf(dest+pos, len-pos, ", desc \"%s\"", info->desc);
+    if (info->no_user)
+        pos += snprintf(dest+pos, len-pos, ", no-user");
+    return pos;
+}
+
 DeviceState *qdev_device_add(const char *cmdline)
 {
     DeviceInfo *info;
@@ -120,8 +135,10 @@ DeviceState *qdev_device_add(const char *cmdline)
         return NULL;
     }
     if (strcmp(driver, "?") == 0) {
+        char msg[256];
         for (info = device_info_list; info != NULL; info = info->next) {
-            fprintf(stderr, "name \"%s\", bus %s\n", info->name, info->bus_info->name);
+            qdev_print_devinfo(info, msg, sizeof(msg));
+            fprintf(stderr, "%s\n", msg);
         }
         return NULL;
     }
-- 
1.6.2.5

[Qemu-devel] [PATCH 2/2] qdev/core: add monitor command to list all drivers

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/qdev.c |   11 +++++++++++
 hw/qdev.h |    1 +
 monitor.c |    2 ++
 3 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 6f05232..9488dba 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -590,3 +590,14 @@ void do_info_qtree(Monitor *mon)
     if (main_system_bus)
         qbus_print(mon, main_system_bus, 0);
 }
+
+void do_info_qdrv(Monitor *mon)
+{
+    DeviceInfo *info;
+    char msg[256];
+
+    for (info = device_info_list; info != NULL; info = info->next) {
+        qdev_print_devinfo(info, msg, sizeof(msg));
+        monitor_printf(mon, "%s\n", msg);
+    }
+}
diff --git a/hw/qdev.h b/hw/qdev.h
index af2ee0f..b342afb 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -143,6 +143,7 @@ BusState *qbus_create(BusInfo *info, DeviceState *parent, const char *name);
 /*** monitor commands ***/
 
 void do_info_qtree(Monitor *mon);
+void do_info_qdrv(Monitor *mon);
 
 /*** qdev-properties.c ***/
 
diff --git a/monitor.c b/monitor.c
index 99c174c..9093254 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1882,6 +1882,8 @@ static const mon_cmd_t info_cmds[] = {
       "", "show balloon information" },
     { "qtree", "", do_info_qtree,
       "", "show device tree" },
+    { "qdrv", "", do_info_qdrv,
+      "", "show qdev driver list" },
     { NULL, NULL, },
 };
 
-- 
1.6.2.5

Re: [Qemu-devel] [PATCH 2/2] qdev/core: add monitor command to list all drivers

[Avi Kivity]

On 07/29/2009 02:12 PM, Gerd Hoffmann wrote:
>
> diff --git a/monitor.c b/monitor.c
> index 99c174c..9093254 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1882,6 +1882,8 @@ static const mon_cmd_t info_cmds[] = {
>         "", "show balloon information" },
>       { "qtree", "", do_info_qtree,
>         "", "show device tree" },
> +    { "qdrv", "", do_info_qdrv,
> +      "", "show qdev driver list" },
>       { NULL, NULL, },
>   };
>    

Drivers are guest things.  You're listing device models, not drivers.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 2/2] qdev/core: add monitor command to list all drivers

[Gerd Hoffmann]

On 08/02/09 16:10, Avi Kivity wrote:
> On 07/29/2009 02:12 PM, Gerd Hoffmann wrote:
>>
>> diff --git a/monitor.c b/monitor.c
>> index 99c174c..9093254 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -1882,6 +1882,8 @@ static const mon_cmd_t info_cmds[] = {
>> "", "show balloon information" },
>> { "qtree", "", do_info_qtree,
>> "", "show device tree" },
>> + { "qdrv", "", do_info_qdrv,
>> + "", "show qdev driver list" },
>> { NULL, NULL, },
>> };
>
> Drivers are guest things. You're listing device models, not drivers.

Strictly speaking you are correct.  But qemu is quite sloppy here and 
calls the device models drivers (almost?) everythere, so I'd tend to 
stick with it for consistency ...

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 2/2] qdev/core: add monitor command to list all drivers

[Avi Kivity]

On 08/03/2009 01:05 PM, Gerd Hoffmann wrote:
> On 08/02/09 16:10, Avi Kivity wrote:
>> On 07/29/2009 02:12 PM, Gerd Hoffmann wrote:
>>>
>>> diff --git a/monitor.c b/monitor.c
>>> index 99c174c..9093254 100644
>>> --- a/monitor.c
>>> +++ b/monitor.c
>>> @@ -1882,6 +1882,8 @@ static const mon_cmd_t info_cmds[] = {
>>> "", "show balloon information" },
>>> { "qtree", "", do_info_qtree,
>>> "", "show device tree" },
>>> + { "qdrv", "", do_info_qdrv,
>>> + "", "show qdev driver list" },
>>> { NULL, NULL, },
>>> };
>>
>> Drivers are guest things. You're listing device models, not drivers.
>
> Strictly speaking you are correct.  But qemu is quite sloppy here and 
> calls the device models drivers (almost?) everythere, so I'd tend to 
> stick with it for consistency ...

Consistently incorrect?

Given that it's an external ABI, let's not be sloppy here.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 1/2] qdev: factor out qdev_print_devinfo.

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/qdev.c |   19 ++++++++++++++++++-
>  1 files changed, 18 insertions(+), 1 deletions(-)
>
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 479eb72..6f05232 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -105,6 +105,21 @@ DeviceState *qdev_create(BusState *bus, const char *name)
>      return dev;
>  }
>  
> +static int qdev_print_devinfo(DeviceInfo *info, char *dest, int len)
> +{
> +    int pos = 0;
> +
> +    pos += snprintf(dest+pos, len-pos, "name \"%s\", bus %s",
> +                    info->name, info->bus_info->name);
> +    if (info->alias)
> +        pos += snprintf(dest+pos, len-pos, ", alias \"%s\"", info->alias);
> +    if (info->desc)
> +        pos += snprintf(dest+pos, len-pos, ", desc \"%s\"", info->desc);
> +    if (info->no_user)
> +        pos += snprintf(dest+pos, len-pos, ", no-user");
> +    return pos;
> +}
> +

Isn't len-pos vulnerable to underflow here?  The formal parameter type
is size_t...

[...]

Re: [Qemu-devel] [PATCH 1/2] qdev: factor out qdev_print_devinfo.

[Gerd Hoffmann]

On 08/01/09 01:44, Markus Armbruster wrote:
> Gerd Hoffmann<kraxel@redhat.com>  writes:
>
>> Signed-off-by: Gerd Hoffmann<kraxel@redhat.com>
>> ---
>>   hw/qdev.c |   19 ++++++++++++++++++-
>>   1 files changed, 18 insertions(+), 1 deletions(-)
>>
>> diff --git a/hw/qdev.c b/hw/qdev.c
>> index 479eb72..6f05232 100644
>> --- a/hw/qdev.c
>> +++ b/hw/qdev.c
>> @@ -105,6 +105,21 @@ DeviceState *qdev_create(BusState *bus, const char *name)
>>       return dev;
>>   }
>>
>> +static int qdev_print_devinfo(DeviceInfo *info, char *dest, int len)
>> +{
>> +    int pos = 0;
>> +
>> +    pos += snprintf(dest+pos, len-pos, "name \"%s\", bus %s",
>> +                    info->name, info->bus_info->name);
>> +    if (info->alias)
>> +        pos += snprintf(dest+pos, len-pos, ", alias \"%s\"", info->alias);
>> +    if (info->desc)
>> +        pos += snprintf(dest+pos, len-pos, ", desc \"%s\"", info->desc);
>> +    if (info->no_user)
>> +        pos += snprintf(dest+pos, len-pos, ", no-user");
>> +    return pos;
>> +}
>> +
>
> Isn't len-pos vulnerable to underflow here?  The formal parameter type
> is size_t...
>
> [...]

Huh?  You mean you want be able to pass a buffer larger than 2^31 to 
that function?

cheers
   Gerd

Re: [Qemu-devel] [PATCH 1/2] qdev: factor out qdev_print_devinfo.

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

> On 08/01/09 01:44, Markus Armbruster wrote:
>> Gerd Hoffmann<kraxel@redhat.com>  writes:
>>
>>> Signed-off-by: Gerd Hoffmann<kraxel@redhat.com>
>>> ---
>>>   hw/qdev.c |   19 ++++++++++++++++++-
>>>   1 files changed, 18 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/hw/qdev.c b/hw/qdev.c
>>> index 479eb72..6f05232 100644
>>> --- a/hw/qdev.c
>>> +++ b/hw/qdev.c
>>> @@ -105,6 +105,21 @@ DeviceState *qdev_create(BusState *bus, const char *name)
>>>       return dev;
>>>   }
>>>
>>> +static int qdev_print_devinfo(DeviceInfo *info, char *dest, int len)
>>> +{
>>> +    int pos = 0;
>>> +
>>> +    pos += snprintf(dest+pos, len-pos, "name \"%s\", bus %s",
>>> +                    info->name, info->bus_info->name);
>>> +    if (info->alias)
>>> +        pos += snprintf(dest+pos, len-pos, ", alias \"%s\"", info->alias);
>>> +    if (info->desc)
>>> +        pos += snprintf(dest+pos, len-pos, ", desc \"%s\"", info->desc);
>>> +    if (info->no_user)
>>> +        pos += snprintf(dest+pos, len-pos, ", no-user");
>>> +    return pos;
>>> +}
>>> +
>>
>> Isn't len-pos vulnerable to underflow here?  The formal parameter type
>> is size_t...
>>
>> [...]
>
> Huh?  You mean you want be able to pass a buffer larger than 2^31 to
> that function?
>
> cheers
>   Gerd

snprintf() returns length of output.  This may exceed its buffer size
argument.  Therefore, pos can grow beyond len, and then len-pos becomes
negative.  Parameter passing casts that to size_t, and snprintf()
happily writes beyond the buffer.

Re: [Qemu-devel] [PATCH 1/2] qdev: factor out qdev_print_devinfo.

[Gerd Hoffmann]

On 08/03/09 10:24, Markus Armbruster wrote:
> Gerd Hoffmann<kraxel@redhat.com>  writes:
>
>> On 08/01/09 01:44, Markus Armbruster wrote:
>>> Gerd Hoffmann<kraxel@redhat.com>   writes:
>>>
>>>> Signed-off-by: Gerd Hoffmann<kraxel@redhat.com>
>>>> ---
>>>>    hw/qdev.c |   19 ++++++++++++++++++-
>>>>    1 files changed, 18 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/hw/qdev.c b/hw/qdev.c
>>>> index 479eb72..6f05232 100644
>>>> --- a/hw/qdev.c
>>>> +++ b/hw/qdev.c
>>>> @@ -105,6 +105,21 @@ DeviceState *qdev_create(BusState *bus, const char *name)
>>>>        return dev;
>>>>    }
>>>>
>>>> +static int qdev_print_devinfo(DeviceInfo *info, char *dest, int len)
>>>> +{
>>>> +    int pos = 0;
>>>> +
>>>> +    pos += snprintf(dest+pos, len-pos, "name \"%s\", bus %s",
>>>> +                    info->name, info->bus_info->name);
>>>> +    if (info->alias)
>>>> +        pos += snprintf(dest+pos, len-pos, ", alias \"%s\"", info->alias);
>>>> +    if (info->desc)
>>>> +        pos += snprintf(dest+pos, len-pos, ", desc \"%s\"", info->desc);
>>>> +    if (info->no_user)
>>>> +        pos += snprintf(dest+pos, len-pos, ", no-user");
>>>> +    return pos;
>>>> +}
>>>> +
>>> Isn't len-pos vulnerable to underflow here?  The formal parameter type
>>> is size_t...
>>>
>>> [...]
>> Huh?  You mean you want be able to pass a buffer larger than 2^31 to
>> that function?
>>
>> cheers
>>    Gerd
>
> snprintf() returns length of output.  This may exceed its buffer size
> argument.

[ after reading the man page ]

Ah.  In case the buffer doesn't fit it doesn't return the number of 
bytes actually written.  It returns the number of bytes needed to print 
everything.

The logic is wrong then, I'll send a fix.

cheers,
   Gerd