[Qemu-devel] OPT_SIZE parsing

[Qemu-devel] OPT_SIZE parsing

[Ian Molton]

Hi,

Qemu currently is making a bit of a hash of parsing suffixes,

Right now, it has:

T, G, M, and K  which are multiples of 1024 bytes - fair enough

but it also has:

k - 1024  (should be 1000)

and b:

Byte (also wrong)

since its only using a single character, with b taken, theres no way to
represent 'bit' unless I use B, which is a bit, well daft.

Would there be any issues if this was updated to parse
[T,t,G,g,M,m,K,k,][,B,b] type syntax, so we could have things like Kb
and MB for Kbits and Megabytes respectively ?

Re: [Qemu-devel] OPT_SIZE parsing

[Paul Brook]

On Tuesday 17 November 2009, Ian Molton wrote:
> Hi,
> 
> Qemu currently is making a bit of a hash of parsing suffixes,
> 
> Right now, it has:
> 
> T, G, M, and K  which are multiples of 1024 bytes - fair enough
> 
> but it also has:
> 
> k - 1024  (should be 1000)
> 
> and b:
> 
> Byte (also wrong)
> 
> since its only using a single character, with b taken, theres no way to
> represent 'bit' unless I use B, which is a bit, well daft.
> 
> Would there be any issues if this was updated to parse
> [T,t,G,g,M,m,K,k,][,B,b] type syntax, so we could have things like Kb
> and MB for Kbits and Megabytes respectively ?

When do we ever have a value that can be specified as both bits and bytes? I 
don't think it makes sense to specify this. The fact that we accept a "b" 
suffix at all is suspicious.
For the magnitude IMO only sane thing to do is ignore the case, and decide 
whether we're using binary or decimal multipliers. Remember that if you're 
being pedantic then "m" should be 0.001.

Paul

Re: [Qemu-devel] OPT_SIZE parsing

[Ian Molton]

[-- Attachment #1: Type: text/plain, Size: 830 bytes --]

Paul Brook wrote:
> On Tuesday 17 November 2009, Ian Molton wrote:

> When do we ever have a value that can be specified as both bits and bytes? I 
> don't think it makes sense to specify this. The fact that we accept a "b" 
> suffix at all is suspicious.

Well, entropy is often measured in 'bits' and I'd like to be able to
specify the entropy rate limit in bits/s or kbits/s when I submit the
virtio-rng driver.

> For the magnitude IMO only sane thing to do is ignore the case, and decide 
> whether we're using binary or decimal multipliers. Remember that if you're 
> being pedantic then "m" should be 0.001.

Quite. I'm happy with binary multipliers, personally.

I've cooked up this patch (attached) to add a SIZE property to qdevs.
I've kept the same semantics as the OPT_SIZE parser for now.

TIA for the review.

-Ian


[-- Attachment #2: 0001-Add-SIZE-type-to-qdev-properties.patch --]
[-- Type: text/x-patch, Size: 3643 bytes --]

>From 64dc4afcc0a6ba0559e918ff75229133b8887eee Mon Sep 17 00:00:00 2001
From: Ian Molton <ian.molton@collabora.co.uk>
Date: Tue, 17 Nov 2009 14:10:10 +0000
Subject: [PATCH] Add SIZE type to qdev properties

    	This patch adds a 'SIZE' type property to those available to qdevs.
    It is the analogue of the OPT_SIZE property for options.

    Signed-off-by: Ian Molton <ian.molton@collabora.co.uk>
---
 hw/qdev-properties.c |   59 ++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/qdev.h            |    4 +++
 2 files changed, 63 insertions(+), 0 deletions(-)

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index bda6699..4899c27 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -194,6 +194,65 @@ PropertyInfo qdev_prop_hex64 = {
     .print = print_hex64,
 };
 
+/* --- 64bit unsigned int 'size' type --- */
+
+static int parse_size(DeviceState *dev, Property *prop, const char *str)
+{
+    uint64_t *ptr = qdev_get_prop_ptr(dev, prop);
+    char *postfix;
+    double sizef;
+
+    if (str != NULL) {
+        sizef = strtod(str, &postfix);
+        switch (*postfix) {
+        case 'T':
+            sizef *= 1024;
+        case 'G':
+            sizef *= 1024;
+        case 'M':
+            sizef *= 1024;
+        case 'K':
+        case 'k':
+            sizef *= 1024;
+        case 'b':
+        case '\0':
+            *ptr = (uint64_t) sizef;
+            break;
+        default:
+            fprintf(stderr, "Option '%s' needs size as parameter\n", prop->name);
+            fprintf(stderr, "You may use k, M, G or T suffixes for "
+                    "kilobytes, megabytes, gigabytes and terabytes.\n");
+            return -1;
+        }
+    } else {
+        fprintf(stderr, "Option '%s' needs a parameter\n", prop->name);
+        return -1;
+    }
+
+    return 0;
+}
+
+static int print_size(DeviceState *dev, Property *prop, char *dest, size_t len)
+{
+    uint64_t *ptr = qdev_get_prop_ptr(dev, prop);
+    char suffixes[] = {'T', 'G', 'M', 'K', 'B'};
+    int i;
+    uint64_t div;
+
+    for(div = (long int)1 << 40 ;
+        !(*ptr / div) ; div >>= 10, i++);
+
+    return snprintf(dest, len, "%0.03f%c", (double)*ptr/div, suffixes[i]);
+}
+
+PropertyInfo qdev_prop_size = {
+    .name  = "size",
+    .type  = PROP_TYPE_SIZE,
+    .size  = sizeof(uint64_t),
+    .parse = parse_size,
+    .print = print_size,
+};
+
 /* --- string --- */
 
 static int parse_string(DeviceState *dev, Property *prop, const char *str)
diff --git a/hw/qdev.h b/hw/qdev.h
index 41642ee..5521093 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -74,6 +74,7 @@ enum PropertyType {
     PROP_TYPE_UINT32,
     PROP_TYPE_INT32,
     PROP_TYPE_UINT64,
+    PROP_TYPE_SIZE,
     PROP_TYPE_TADDR,
     PROP_TYPE_MACADDR,
     PROP_TYPE_DRIVE,
@@ -189,6 +190,7 @@ extern PropertyInfo qdev_prop_int32;
 extern PropertyInfo qdev_prop_uint64;
 extern PropertyInfo qdev_prop_hex32;
 extern PropertyInfo qdev_prop_hex64;
+extern PropertyInfo qdev_prop_size;
 extern PropertyInfo qdev_prop_string;
 extern PropertyInfo qdev_prop_chr;
 extern PropertyInfo qdev_prop_ptr;
@@ -226,6 +228,8 @@ extern PropertyInfo qdev_prop_pci_devfn;
     DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_hex32, uint32_t)
 #define DEFINE_PROP_HEX64(_n, _s, _f, _d)                       \
     DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_hex64, uint64_t)
+#define DEFINE_PROP_SIZE(_n, _s, _f, _d)                       \
+    DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_size, uint64_t)
 #define DEFINE_PROP_PCI_DEVFN(_n, _s, _f, _d)                   \
     DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_pci_devfn, uint32_t)
 
-- 
1.6.5

Re: [Qemu-devel] OPT_SIZE parsing

[Gerd Hoffmann]

On 11/17/09 15:23, Ian Molton wrote:
> I've cooked up this patch (attached) to add a SIZE property to qdevs.
> I've kept the same semantics as the OPT_SIZE parser for now.

The error message should be adapted (s/Option/Property/ at least).

Maybe also create a common function for parsing called by both 
parse_size() and parse_option_size() to make sure OPT_SIZE and the new 
size property accept the same syntax?

Otherwise it looks fine to me.

cheers,
   Gerd

Re: [Qemu-devel] OPT_SIZE parsing

[Ian Molton]

Gerd Hoffmann wrote:
> On 11/17/09 15:23, Ian Molton wrote:
>> I've cooked up this patch (attached) to add a SIZE property to qdevs.
>> I've kept the same semantics as the OPT_SIZE parser for now.
> 
> The error message should be adapted (s/Option/Property/ at least).

Fixed locally.

> Maybe also create a common function for parsing called by both
> parse_size() and parse_option_size() to make sure OPT_SIZE and the new
> size property accept the same syntax?

The thought crossed my mind, but then I thought that as none of the
other parsers share common code (yet), it didn't make sense.

I can cook up another patch that allows the option parser to hook into
the property parsing functions, or vice-versa, if you like?

> Otherwise it looks fine to me.

Cool - whats the submission procedure? I can make a git branch publicly
available or I can send patches to someone...

-Ian

Re: [Qemu-devel] OPT_SIZE parsing

[Gerd Hoffmann]

On 11/17/09 19:11, Ian Molton wrote:
> Gerd Hoffmann wrote:
>> Maybe also create a common function for parsing called by both
>> parse_size() and parse_option_size() to make sure OPT_SIZE and the new
>> size property accept the same syntax?
>
> The thought crossed my mind, but then I thought that as none of the
> other parsers share common code (yet), it didn't make sense.

Well, when the parser is basically a simple strtoull() call it doesn't 
make sense.  For the size and the postfix processing it makes sense 
IMHO, they easily get out of sync otherwise.  For a 'bool' property (if 
qdev gets one some day) it would make sense too I think.

> I can cook up another patch that allows the option parser to hook into
> the property parsing functions, or vice-versa, if you like?

Given that qemu-option.c is also used by the tools (qemu-img, ...) it 
will be much easier to have qdev use a option parser function than the 
other way around.

>> Otherwise it looks fine to me.
>
> Cool - whats the submission procedure? I can make a git branch publicly
> available or I can send patches to someone...

Just send it to the list, one of the maintainers with commit access 
(most likely anthony) should pick it up.  If it doesn't work try 
resending after 2-3 weeks.

cheers,
   Gerd