Re: [Qemu-devel] [PATCH 01/10] Introduce qmisc module

[Luiz Capitulino]

On Thu, 15 Oct 2009 09:02:48 -0500
Anthony Liguori <address@hidden> 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 <address@hidden>
> > ---
> >  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 <address@hidden>
> > + *
> > + * 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. :))