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

[Anthony Liguori <anthony@codemonkey.ws>]

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