| Message ID | 1255037747-3340-2-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | Under Review |
| Headers | show |
Luiz Capitulino wrote: > This module provides miscellania QObject functions. > > Currently it exports qobject_from_fmt(), which is somewhat > based on Python's Py_BuildValue() function. It is capable of > creating QObjects from a specified string format. > > For example, to create a QDict with mixed data-types one > could do: > > QObject *obj = qobject_from_fmt("{ s: [ i, s ], s: i }", ... ); > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > --- > Makefile | 2 +- > qmisc.c | 222 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > qmisc.h | 19 +++++ > 3 files changed, 242 insertions(+), 1 deletions(-) > create mode 100644 qmisc.c > create mode 100644 qmisc.h > > diff --git a/Makefile b/Makefile > index d96fb4b..182f176 100644 > --- a/Makefile > +++ b/Makefile > @@ -100,7 +100,7 @@ obj-y += buffered_file.o migration.o migration-tcp.o net.o qemu-sockets.o > obj-y += qemu-char.o aio.o net-checksum.o savevm.o > obj-y += msmouse.o ps2.o > obj-y += qdev.o qdev-properties.o ssi.o > -obj-y += qint.o qstring.o qdict.o qlist.o qemu-config.o > +obj-y += qint.o qstring.o qdict.o qlist.o qmisc.o qemu-config.o > > obj-$(CONFIG_BRLAPI) += baum.o > obj-$(CONFIG_WIN32) += tap-win32.o > diff --git a/qmisc.c b/qmisc.c > new file mode 100644 > index 0000000..42b6f22 > --- /dev/null > +++ b/qmisc.c > @@ -0,0 +1,222 @@ > +/* > + * Misc QObject functions. > + * > + * Copyright (C) 2009 Red Hat Inc. > + * > + * Authors: > + * Luiz Capitulino <lcapitulino@redhat.com> > + * > + * This work is licensed under the terms of the GNU GPL, version 2. See > + * the COPYING file in the top-level directory. > + */ > +#include "qmisc.h" > +#include "qint.h" > +#include "qlist.h" > +#include "qdict.h" > +#include "qstring.h" > +#include "qobject.h" > +#include "qemu-common.h" > + > +/* > + * qobject_from_fmt() and related functions are based on the Python's > + * Py_BuildValue() and are subject to the Python Software Foundation > + * License Version 2. > + * > + * Copyright (c) 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009 Python > + * Software Foundation. > + */ > If we're introducing third-party code under a new license, we need to update the top-level LICENSE file. I took a brief look and it wasn't immediately clear that this license is GPL compatible. According to the FSF, certain versions of this license are incompatible and some are compatible. I think it would have been better to just write something from scratch... > + case '[': > + return do_mklist(fmt, args, ']', count_format(*fmt, ']')); > Because this is bizarre. It looks ahead to count the number of arguments which is a very strange way to parse something like this. Why not a simple recursive decent parser? > +/** > + * qobject_from_fmt(): build QObjects from a specified format. > + * > + * Valid characters of the format: > + * > + * i integer, map to QInt > + * s string, map to QString > + * [] list, map to QList > + * {} dictionary, map to QDict > + * > + * Examples: > + * > + * - Create a QInt > + * > + * qobject_from_fmt("i", 42); > + * > + * - Create a QList of QStrings > + * > + * qobject_from_fmt("[ i, i, i ]", 0, 1 , 2); > + * > + * - Create a QDict with mixed data-types > + * > + * qobject_from_fmt("{ s: [ i, s ], s: i }", ... ); > + * > + * Return a strong reference to a QObject on success, NULL otherwise. > + */ > But my real objection is that we should make this "{%s: [%d, %s], %s: %d}" so that we can mark it as a printf formatted function and get type checking. You'll probably have to support both "%d" and "%" PRId64 for sanity sake. Regards, Anthony Liguori
On Thu, 15 Oct 2009 09:02:48 -0500 Anthony Liguori <anthony@codemonkey.ws> wrote: > Luiz Capitulino wrote: > > This module provides miscellania QObject functions. > > > > Currently it exports qobject_from_fmt(), which is somewhat > > based on Python's Py_BuildValue() function. It is capable of > > creating QObjects from a specified string format. > > > > For example, to create a QDict with mixed data-types one > > could do: > > > > QObject *obj = qobject_from_fmt("{ s: [ i, s ], s: i }", ... ); > > > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > > --- > > Makefile | 2 +- > > qmisc.c | 222 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > qmisc.h | 19 +++++ > > 3 files changed, 242 insertions(+), 1 deletions(-) > > create mode 100644 qmisc.c > > create mode 100644 qmisc.h > > > > diff --git a/Makefile b/Makefile > > index d96fb4b..182f176 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -100,7 +100,7 @@ obj-y += buffered_file.o migration.o migration-tcp.o net.o qemu-sockets.o > > obj-y += qemu-char.o aio.o net-checksum.o savevm.o > > obj-y += msmouse.o ps2.o > > obj-y += qdev.o qdev-properties.o ssi.o > > -obj-y += qint.o qstring.o qdict.o qlist.o qemu-config.o > > +obj-y += qint.o qstring.o qdict.o qlist.o qmisc.o qemu-config.o > > > > obj-$(CONFIG_BRLAPI) += baum.o > > obj-$(CONFIG_WIN32) += tap-win32.o > > diff --git a/qmisc.c b/qmisc.c > > new file mode 100644 > > index 0000000..42b6f22 > > --- /dev/null > > +++ b/qmisc.c > > @@ -0,0 +1,222 @@ > > +/* > > + * Misc QObject functions. > > + * > > + * Copyright (C) 2009 Red Hat Inc. > > + * > > + * Authors: > > + * Luiz Capitulino <lcapitulino@redhat.com> > > + * > > + * This work is licensed under the terms of the GNU GPL, version 2. See > > + * the COPYING file in the top-level directory. > > + */ > > +#include "qmisc.h" > > +#include "qint.h" > > +#include "qlist.h" > > +#include "qdict.h" > > +#include "qstring.h" > > +#include "qobject.h" > > +#include "qemu-common.h" > > + > > +/* > > + * qobject_from_fmt() and related functions are based on the Python's > > + * Py_BuildValue() and are subject to the Python Software Foundation > > + * License Version 2. > > + * > > + * Copyright (c) 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009 Python > > + * Software Foundation. > > + */ > > > > If we're introducing third-party code under a new license, we need to > update the top-level LICENSE file. I took a brief look and it wasn't > immediately clear that this license is GPL compatible. According to the > FSF, certain versions of this license are incompatible and some are > compatible. I think it would have been better to just write something > from scratch... According to the Python's LICENSE file it's compatible since 2001 (2.0.1 release). > > + case '[': > > + return do_mklist(fmt, args, ']', count_format(*fmt, ']')); > > > > Because this is bizarre. It looks ahead to count the number of > arguments which is a very strange way to parse something like this. > > Why not a simple recursive decent parser? I could try it, but I think this is going to take some time as I would have to read more about it. I thought the Python's implementation was a good idea as we're short in time and it was easy to adapt and is widely used in production. > > +/** > > + * qobject_from_fmt(): build QObjects from a specified format. > > + * > > + * Valid characters of the format: > > + * > > + * i integer, map to QInt > > + * s string, map to QString > > + * [] list, map to QList > > + * {} dictionary, map to QDict > > + * > > + * Examples: > > + * > > + * - Create a QInt > > + * > > + * qobject_from_fmt("i", 42); > > + * > > + * - Create a QList of QStrings > > + * > > + * qobject_from_fmt("[ i, i, i ]", 0, 1 , 2); > > + * > > + * - Create a QDict with mixed data-types > > + * > > + * qobject_from_fmt("{ s: [ i, s ], s: i }", ... ); > > + * > > + * Return a strong reference to a QObject on success, NULL otherwise. > > + */ > > > > But my real objection is that we should make this "{%s: [%d, %s], %s: > %d}" so that we can mark it as a printf formatted function and get type > checking. You'll probably have to support both "%d" and "%" PRId64 for > sanity sake. Trivial to do if we ignore the '%' characters. :))
Luiz Capitulino wrote: >>> +/** >>> + * qobject_from_fmt(): build QObjects from a specified format. >>> + * >>> + * Valid characters of the format: >>> + * >>> + * i integer, map to QInt >>> + * s string, map to QString >>> + * [] list, map to QList >>> + * {} dictionary, map to QDict >>> + * >>> + * Examples: >>> + * >>> + * - Create a QInt >>> + * >>> + * qobject_from_fmt("i", 42); >>> + * >>> + * - Create a QList of QStrings >>> + * >>> + * qobject_from_fmt("[ i, i, i ]", 0, 1 , 2); >>> + * >>> + * - Create a QDict with mixed data-types >>> + * >>> + * qobject_from_fmt("{ s: [ i, s ], s: i }", ... ); >>> + * >>> + * Return a strong reference to a QObject on success, NULL otherwise. >>> + */ >>> >>> >> But my real objection is that we should make this "{%s: [%d, %s], %s: >> %d}" so that we can mark it as a printf formatted function and get type >> checking. You'll probably have to support both "%d" and "%" PRId64 for >> sanity sake. >> > > Trivial to do if we ignore the '%' characters. :)) > Okay, I'd like to see that and I'd like to see string parsing. We can rewrite the parser later. Regards, Anthony Liguori
On Thu, Oct 15, 2009 at 12:26:22PM -0300, Luiz Capitulino wrote: > On Thu, 15 Oct 2009 09:02:48 -0500 > Anthony Liguori <anthony@codemonkey.ws> wrote: > > > > > If we're introducing third-party code under a new license, we need to > > update the top-level LICENSE file. I took a brief look and it wasn't > > immediately clear that this license is GPL compatible. According to the > > FSF, certain versions of this license are incompatible and some are > > compatible. I think it would have been better to just write something > > from scratch... > > According to the Python's LICENSE file it's compatible since 2001 > (2.0.1 release). > > > > + case '[': > > > + return do_mklist(fmt, args, ']', count_format(*fmt, ']')); > > > > > > > Because this is bizarre. It looks ahead to count the number of > > arguments which is a very strange way to parse something like this. > > > > Why not a simple recursive decent parser? > > I could try it, but I think this is going to take some time as > I would have to read more about it. > > I thought the Python's implementation was a good idea as we're short > in time and it was easy to adapt and is widely used in production. There are at least 6 standalone, pure C [1] json parsers available already, some of which let you do json formatting too. So writing a new parser, or untangling one from python seems like more trouble than its worth to me. Likewise for generating formatted JSON output. For QMP experimentation in libvirt I've started off by just grabbing the json.h & json.c files from an existing impl http://mjson.svn.sourceforge.net/viewvc/mjson/trunk/src/ which are LGPLv2+ licensed, and copied them into libvirt source tree. They provide quite a nice simple API for both parsing & formatting and integrate very easily with our codebase. Regards, Daniel [1] See http://json.org/
On Thu, Oct 15, 2009 at 05:39:36PM +0100, Daniel P. Berrange wrote: > On Thu, Oct 15, 2009 at 12:26:22PM -0300, Luiz Capitulino wrote: > > On Thu, 15 Oct 2009 09:02:48 -0500 > > Anthony Liguori <anthony@codemonkey.ws> wrote: > > > > > > > > If we're introducing third-party code under a new license, we need to > > > update the top-level LICENSE file. I took a brief look and it wasn't > > > immediately clear that this license is GPL compatible. According to the > > > FSF, certain versions of this license are incompatible and some are > > > compatible. I think it would have been better to just write something > > > from scratch... > > > > According to the Python's LICENSE file it's compatible since 2001 > > (2.0.1 release). > > > > > > + case '[': > > > > + return do_mklist(fmt, args, ']', count_format(*fmt, ']')); > > > > > > > > > > Because this is bizarre. It looks ahead to count the number of > > > arguments which is a very strange way to parse something like this. > > > > > > Why not a simple recursive decent parser? > > > > I could try it, but I think this is going to take some time as > > I would have to read more about it. > > > > I thought the Python's implementation was a good idea as we're short > > in time and it was easy to adapt and is widely used in production. > > There are at least 6 standalone, pure C [1] json parsers available > already, some of which let you do json formatting too. So writing a > new parser, or untangling one from python seems like more trouble than > its worth to me. Likewise for generating formatted JSON output. Opps. Ignore me. I'm replying to totally the wrong email / context here :-) Daniel
On Thu, 15 Oct 2009 10:35:16 -0500 Anthony Liguori <anthony@codemonkey.ws> wrote: > Luiz Capitulino wrote: > >>> +/** > >>> + * qobject_from_fmt(): build QObjects from a specified format. > >>> + * > >>> + * Valid characters of the format: > >>> + * > >>> + * i integer, map to QInt > >>> + * s string, map to QString > >>> + * [] list, map to QList > >>> + * {} dictionary, map to QDict > >>> + * > >>> + * Examples: > >>> + * > >>> + * - Create a QInt > >>> + * > >>> + * qobject_from_fmt("i", 42); > >>> + * > >>> + * - Create a QList of QStrings > >>> + * > >>> + * qobject_from_fmt("[ i, i, i ]", 0, 1 , 2); > >>> + * > >>> + * - Create a QDict with mixed data-types > >>> + * > >>> + * qobject_from_fmt("{ s: [ i, s ], s: i }", ... ); > >>> + * > >>> + * Return a strong reference to a QObject on success, NULL otherwise. > >>> + */ > >>> > >>> > >> But my real objection is that we should make this "{%s: [%d, %s], %s: > >> %d}" so that we can mark it as a printf formatted function and get type > >> checking. You'll probably have to support both "%d" and "%" PRId64 for > >> sanity sake. > >> > > > > Trivial to do if we ignore the '%' characters. :)) > > > Okay, I'd like to see that and I'd like to see string parsing. We can > rewrite the parser later. Great! I have some concerns about string parsing though, can we make it very simple? Like, only valid as keys and not expect anything fancy inside the string delimiters? Otherwise I think it would be easier to make it part of a grammar, which sounds like a good future improvement.
On Thu, 15 Oct 2009 17:39:36 +0100 "Daniel P. Berrange" <berrange@redhat.com> wrote: > On Thu, Oct 15, 2009 at 12:26:22PM -0300, Luiz Capitulino wrote: > > On Thu, 15 Oct 2009 09:02:48 -0500 > > Anthony Liguori <anthony@codemonkey.ws> wrote: > > > > > > > > If we're introducing third-party code under a new license, we need to > > > update the top-level LICENSE file. I took a brief look and it wasn't > > > immediately clear that this license is GPL compatible. According to the > > > FSF, certain versions of this license are incompatible and some are > > > compatible. I think it would have been better to just write something > > > from scratch... > > > > According to the Python's LICENSE file it's compatible since 2001 > > (2.0.1 release). > > > > > > + case '[': > > > > + return do_mklist(fmt, args, ']', count_format(*fmt, ']')); > > > > > > > > > > Because this is bizarre. It looks ahead to count the number of > > > arguments which is a very strange way to parse something like this. > > > > > > Why not a simple recursive decent parser? > > > > I could try it, but I think this is going to take some time as > > I would have to read more about it. > > > > I thought the Python's implementation was a good idea as we're short > > in time and it was easy to adapt and is widely used in production. > > There are at least 6 standalone, pure C [1] json parsers available > already, some of which let you do json formatting too. So writing a > new parser, or untangling one from python seems like more trouble than > its worth to me. Likewise for generating formatted JSON output. Not the right context but I was going to post about this soon, so I think this is a good opportunity to talk about it. I didn't look at all available parsers from json.org, but this one: http://fara.cs.uni-potsdam.de/~jsg/json_parser/ Seems interesting. Anthony, are you ok in using external implementations like that if they meet our requirements?
Luiz Capitulino wrote: >> >> Okay, I'd like to see that and I'd like to see string parsing. We can >> rewrite the parser later. >> > > Great! > > I have some concerns about string parsing though, can we make it very > simple? Like, only valid as keys and not expect anything fancy inside > the string delimiters? > Yup. My main concern is that I don't want to convert all of the various commands only to have to revisit them to introduce a new formatting syntax. > Otherwise I think it would be easier to make it part of a grammar, > which sounds like a good future improvement. > Regards, Anthony Liguori
Luiz Capitulino wrote: > Not the right context but I was going to post about this soon, so > I think this is a good opportunity to talk about it. > > I didn't look at all available parsers from json.org, but this one: > > http://fara.cs.uni-potsdam.de/~jsg/json_parser/ > > Seems interesting. > > Anthony, are you ok in using external implementations like that > if they meet our requirements? > If it's a library, it should be widely available (iow, packaged on all major distros) and there should be binaries available for Windows. Otherwise, pulling the code into the tree isn't so bad provided that it's not huge. Regards, Anthony Liguori
Luiz Capitulino wrote: > Great! > > I have some concerns about string parsing though, can we make it very > simple? Like, only valid as keys and not expect anything fancy inside > the string delimiters? > > Otherwise I think it would be easier to make it part of a grammar, > which sounds like a good future improvement. > You know, once we have a json parser, we'll probably just want to turn this into an asprintf() that feeds into the json parser. So just make sure that the format is valid json. In fact, it may be better to name this qobject_from_json(). Regards, Anthony Liguori
diff --git a/Makefile b/Makefile index d96fb4b..182f176 100644 --- a/Makefile +++ b/Makefile @@ -100,7 +100,7 @@ obj-y += buffered_file.o migration.o migration-tcp.o net.o qemu-sockets.o obj-y += qemu-char.o aio.o net-checksum.o savevm.o obj-y += msmouse.o ps2.o obj-y += qdev.o qdev-properties.o ssi.o -obj-y += qint.o qstring.o qdict.o qlist.o qemu-config.o +obj-y += qint.o qstring.o qdict.o qlist.o qmisc.o qemu-config.o obj-$(CONFIG_BRLAPI) += baum.o obj-$(CONFIG_WIN32) += tap-win32.o diff --git a/qmisc.c b/qmisc.c new file mode 100644 index 0000000..42b6f22 --- /dev/null +++ b/qmisc.c @@ -0,0 +1,222 @@ +/* + * Misc QObject functions. + * + * Copyright (C) 2009 Red Hat Inc. + * + * Authors: + * Luiz Capitulino <lcapitulino@redhat.com> + * + * This work is licensed under the terms of the GNU GPL, version 2. See + * the COPYING file in the top-level directory. + */ +#include "qmisc.h" +#include "qint.h" +#include "qlist.h" +#include "qdict.h" +#include "qstring.h" +#include "qobject.h" +#include "qemu-common.h" + +/* + * qobject_from_fmt() and related functions are based on the Python's + * Py_BuildValue() and are subject to the Python Software Foundation + * License Version 2. + * + * Copyright (c) 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009 Python + * Software Foundation. + */ +static QObject *build_qobject(const char **fmt, va_list *args); + +static QObject *do_mkdict(const char **fmt, va_list *args, int endchar, + int nr_elems) +{ + int i; + QDict *qdict; + + if (nr_elems < 0) + return NULL; + + qdict = qdict_new(); + + for (i = 0; i < nr_elems; i += 2) { + QString *qs; + QObject *obj, *key; + + key = build_qobject(fmt, args); + if (!key) + goto err; + + obj = build_qobject(fmt, args); + if (!obj) { + qobject_decref(key); + goto err; + } + + qs = qobject_to_qstring(key); + qdict_put_obj(qdict, qstring_get_str(qs), obj); + QDECREF(qs); + } + + while (**fmt == ' ') + (*fmt)++; + + if (**fmt != endchar) + goto err; + + if (endchar) + ++*fmt; + + return QOBJECT(qdict); + +err: + QDECREF(qdict); + return NULL; +} + +static QObject *do_mklist(const char **fmt, va_list *args, int endchar, + int nr_elems) +{ + int i; + QList *qlist; + + if (nr_elems < 0) + return NULL; + + qlist = qlist_new(); + + for (i = 0; i < nr_elems; i++) { + QObject *obj = build_qobject(fmt, args); + if (!obj) + goto err; + qlist_append_obj(qlist, obj); + } + + while (**fmt == ' ') + (*fmt)++; + + if (**fmt != endchar) + goto err; + + if (endchar) + ++*fmt; + + return QOBJECT(qlist); + +err: + QDECREF(qlist); + return NULL; +} + +static int +count_format(const char *format, int endchar) +{ + int count = 0; + int level = 0; + + while (level > 0 || *format != endchar) { + switch (*format) { + case '\0': + /* Premature end */ + return -1; + case '[': + case '{': + if (level == 0) + count++; + level++; + break; + case ']': + case '}': + level--; + break; + case ',': + case ':': + case ' ': + case '\t': + break; + default: + if (level == 0) + count++; + } + format++; + } + return count; +} + +static QObject *build_qobject(const char **fmt, va_list *args) +{ + for (;;) { + switch (*(*fmt)++) { + case 'i': + { + QInt *qi = qint_from_int((int64_t) va_arg(*args, int64_t)); + return QOBJECT(qi); + } + case 's': + { + QString *qs = qstring_from_str((char *) va_arg(*args, char *)); + return QOBJECT(qs); + } + case '[': + return do_mklist(fmt, args, ']', count_format(*fmt, ']')); + case '{': + return do_mkdict(fmt, args, '}', count_format(*fmt, '}')); + case ',': + case ':': + case ' ': + case '\t': + break; + default: + return NULL; + } + } +} + +/** + * qobject_from_fmt(): build QObjects from a specified format. + * + * Valid characters of the format: + * + * i integer, map to QInt + * s string, map to QString + * [] list, map to QList + * {} dictionary, map to QDict + * + * Examples: + * + * - Create a QInt + * + * qobject_from_fmt("i", 42); + * + * - Create a QList of QStrings + * + * qobject_from_fmt("[ i, i, i ]", 0, 1 , 2); + * + * - Create a QDict with mixed data-types + * + * qobject_from_fmt("{ s: [ i, s ], s: i }", ... ); + * + * Return a strong reference to a QObject on success, NULL otherwise. + */ +QObject *qobject_from_fmt(const char *fmt, ...) +{ + va_list args; + QObject *obj; + + va_start(args, fmt); + switch (*fmt) { + case '[': + fmt++; + obj = do_mklist(&fmt, &args, ']', count_format(fmt, ']')); + break; + case '{': + fmt++; + obj = do_mkdict(&fmt, &args, '}', count_format(fmt, '}')); + break; + default: + obj = build_qobject(&fmt, &args); + break; + } + va_end(args); + + return obj; +} diff --git a/qmisc.h b/qmisc.h new file mode 100644 index 0000000..ac481fe --- /dev/null +++ b/qmisc.h @@ -0,0 +1,19 @@ +/* + * QMisc header file. + * + * Copyright (C) 2009 Red Hat Inc. + * + * Authors: + * Luiz Capitulino <lcapitulino@redhat.com> + * + * This work is licensed under the terms of the GNU GPL, version 2. See + * the COPYING file in the top-level directory. + */ +#ifndef QMISC_H +#define QMISC_H + +#include "qobject.h" + +QObject *qobject_from_fmt(const char *fmt, ...); + +#endif /* QMISC_H */
This module provides miscellania QObject functions. Currently it exports qobject_from_fmt(), which is somewhat based on Python's Py_BuildValue() function. It is capable of creating QObjects from a specified string format. For example, to create a QDict with mixed data-types one could do: QObject *obj = qobject_from_fmt("{ s: [ i, s ], s: i }", ... ); Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> --- Makefile | 2 +- qmisc.c | 222 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ qmisc.h | 19 +++++ 3 files changed, 242 insertions(+), 1 deletions(-) create mode 100644 qmisc.c create mode 100644 qmisc.h