diff mbox

OPT_SIZE parsing

Message ID 4B02B1EF.70206@collabora.co.uk
State New
Headers show

Commit Message

Ian Molton Nov. 17, 2009, 2:23 p.m. UTC 
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

Comments

Gerd Hoffmann Nov. 17, 2009, 4:51 p.m. UTC | #1 
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
Ian Molton Nov. 17, 2009, 6:11 p.m. UTC | #2 
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
Gerd Hoffmann Nov. 17, 2009, 7:19 p.m. UTC | #3 
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
diff mbox

Patch

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