[Qemu-devel] Re: [PATCH 2/7] Introduce QError

[Luiz Capitulino]

On Thu, 29 Oct 2009 15:14:12 -0500
Anthony Liguori <address@hidden> wrote:

> 
> > Signed-off-by: Luiz Capitulino <address@hidden>
> > ---
> >  Makefile  |    2 +-
> >  qerror.c  |  240 
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  qerror.h  |   48 ++++++++++++
> >  qobject.h |    1 +
> >  4 files changed, 290 insertions(+), 1 deletions(-)
> >  create mode 100644 qerror.c
> >  create mode 100644 qerror.h
> >
> > diff --git a/Makefile b/Makefile
> > index 813bd0a..325e583 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -125,7 +125,7 @@ obj-y += net.o net-queue.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
> > -obj-y += qint.o qstring.o qdict.o qlist.o qfloat.o qbool.o qjson.o
> > +obj-y += qint.o qstring.o qdict.o qlist.o qfloat.o qbool.o qjson.o qerror.o
> >  obj-y += qemu-config.o
> >
> >  obj-$(CONFIG_BRLAPI) += baum.o
> > diff --git a/qerror.c b/qerror.c
> > new file mode 100644
> > index 0000000..0359d65
> > --- /dev/null
> > +++ b/qerror.c
> > @@ -0,0 +1,240 @@
> > +/*
> > + * QError: QEMU Error data-type.
> > + *
> > + * 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 "qint.h"
> > +#include "qjson.h"
> > +#include "qerror.h"
> > +#include "qstring.h"
> > +#include "sysemu.h"
> > +#include "qemu-common.h"
> > +
> > +static void qerror_destroy_obj(QObject *obj);
> > +
> > +static const QType qerror_type = {
> > +    .code = QTYPE_QERROR,
> > +    .destroy = qerror_destroy_obj,
> > +};
> > +
> > +/**
> > + * The 'desc' member is a printf-like string, the format of the format
> > + * string is:
> > + *
> > + * %(KEY)TYPE
> > + *
> > + * Where KEY is a QDict key and TYPE is the type of its value, KEY and
> > + * its value must be passed to qerror_from_info().
> > + *
> > + * Example:
> > + *
> > + * "foo error on device: %(device)s slot: %(slot_nr)s"
> > + *
> > + * A single percent sign can be printed if followed by a second one,
> > + * for example:
> > + *
> > + * "running out of foo: %(foo)d%%"
> > + *
> > + * Valid types are:
> > + *
> > + * s    (string)
> > + * d    (integer)
> > + */
> > +static QErrorTable qerror_table[] = {
> > +    {
> > +        .code = QERR_UNKNOWN,
> > +        .desc = "unknown error",
> > +    },
> > +};
> > +
> > +/**
> > + * qerror_new(): Create a new QError
> > + *
> > + * Return strong reference.
> > + */
> > +QError *qerror_new(void)
> > +{
> > +    QError *qerr;
> > +
> > +    qerr = qemu_mallocz(sizeof(*qerr));
> > +    QOBJECT_INIT(qerr, &qerror_type);
> > +
> > +    return qerr;
> > +}
> > +
> > +/**
> > + * qerror_from_info(): Create a new QError from error information
> > + *
> > + * The information consists of:
> > + *
> > + * - code: error code
> > + * - file: the file of where the error happend
> > + * - linenr: the line number of where the error happend
> > + * - fmt: JSON printf-like format
> > + * - va: va_list of all arguments
> > + *
> > + * Return strong reference.
> > + */
> > +QError *qerror_from_info(QErrorCode code, const char *file, int linenr,
> > +                         const char *fmt, va_list *va)
> > +{
> >   
> 
> va_list doesn't need to be a pointer.

 Ok, but if this is going to be a public interface, I think it's
better to va_copy() before passing it to qobject_from_json_va() then.

> Shouldn't this take a ... too?

 The problem is that its caller (qemu_error_structed()) also takes
a ..., I don't know how to pass the arguments to a second function with
variadic arguments w/o passing the va_list.

> > diff --git a/qerror.h b/qerror.h
> > new file mode 100644
> > index 0000000..2fd0d58
> > --- /dev/null
> > +++ b/qerror.h
> > @@ -0,0 +1,48 @@
> > +/*
> > + * QError header file.
> > + *
> > + * 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.
> > + */
> > +#ifndef QERROR_H
> > +#define QERROR_H
> > +
> > +#include <stdarg.h>
> > +#include "qobject.h"
> > +
> > +/*
> > + * IMPORTANT: errors numbers must not change after they have been
> > + * added here.
> > + */
> > +typedef enum QErrorCode {
> > +    QERR_UNKNOWN,
> > +    QERR_MAX,
> > +} QErrorCode;
> > +
> > +struct QError;
> > +
> > +typedef struct QErrorTable {
> > +    QErrorCode code;
> > +    const char *desc;
> > +} QErrorTable;
> > +
> > +typedef struct QError {
> > +    QObject_HEAD;
> > +    int linenr;         /* error line number */
> > +    const char *file;   /* error file */
> > +    QObject *data;      /* error specific data */
> > +    const QErrorTable *entry;
> > +} QError;
> > +
> > +QError *qerror_new(void);
> > +QError *qerror_from_info(QErrorCode code, const char *file, int linenr,
> > +                         const char *fmt, va_list *va);
> >   
> 
> I don't know how I feel about the linenr/file bit.  It seems prone to 
> misused when used in a public interface (vs. automatically via a macro).

 This is what happens in practice, qemu_error_structed() is a macro,
it's where the __LINE__/__FILE__ macros are actually used.

 Although there is no problems in being part of a public interface,
qerror_from_info() is a low-level function, it's supposed to be
part of a higher level API.

 The only way to make this the way you suggest, would be to
export qemu_error_structed() from QError someway. But I'd have to
think how to do this, as we have to deal with the Monitor's bits
and the QemuErrorSink API.

 By the way, what contributes to make this worse is the
all-in-a-file Monitor module.. We also have cyclic dependencies
there. I plan to split it, but probably not for 0.12.