[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 07/10] Introduce QError
From: |
Luiz Capitulino |
Subject: |
Re: [Qemu-devel] [PATCH 07/10] Introduce QError |
Date: |
Wed, 18 Nov 2009 15:23:57 -0200 |
On Wed, 18 Nov 2009 16:16:11 +0100
Markus Armbruster <address@hidden> wrote:
> Luiz Capitulino <address@hidden> writes:
[...]
> > +static const char *append_field(QString *outstr, const QError *qerror,
> > + const char *start)
> > +{
> > + QObject *obj;
> > + QDict *qdict;
> > + QString *key_qs;
> > + const char *end, *key;
> > +
> > + if (*start != '%')
> > + parse_error(qerror, '%');
>
> Can't happen, because it gets called only with *start == '%'. Taking
> pointer to the character following the '%' as argument would sidestep
> the issue. But I'm fine with leaving it as is.
It's just an assertion.
> > + start++;
> > + if (*start != '(')
> > + parse_error(qerror, '(');
> > + start++;
> > +
> > + end = strchr(start, ')');
> > + if (!end)
> > + parse_error(qerror, ')');
> > +
> > + key_qs = qstring_from_substr(start, 0, end - start - 1);
> > + key = qstring_get_str(key_qs);
> > +
> > + qdict = qobject_to_qdict(qdict_get(qerror->error, "data"));
> > + obj = qdict_get(qdict, key);
> > + if (!obj) {
> > + qerror_abort(qerror, "key '%s' not found in QDict", key);
> > + }
> > +
> > + switch (qobject_type(obj)) {
> > + case QTYPE_QSTRING:
> > + qstring_append(outstr, qdict_get_str(qdict, key));
> > + break;
> > + case QTYPE_QINT:
> > + qstring_append_int(outstr, qdict_get_int(qdict, key));
> > + break;
> > + default:
> > + qerror_abort(qerror, "invalid type '%c'", qobject_type(obj));
> > + }
> > +
> > + QDECREF(key_qs);
>
> Looks like you create key_qs just because it's a convenient way to
> extract key zero-terminated. Correct?
Yes, as a substring of 'desc', which is passed through 'start'.
[...]
> > diff --git a/qjson.c b/qjson.c
> > index 12e6cf0..60c904d 100644
> > --- a/qjson.c
> > +++ b/qjson.c
> > @@ -224,6 +224,8 @@ static void to_json(const QObject *obj, QString *str)
> > }
> > break;
> > }
> > + case QTYPE_QERROR:
> > + /* XXX: should QError be emitted? */
>
> Pros & cons?
It's probably convenient to have qjson emitting QError, I'm unsure
if we should do that for all kinds of QObjects though.
> > case QTYPE_NONE:
> > break;
> > }
> > diff --git a/qobject.h b/qobject.h
> > index 2270ec1..07de211 100644
> > --- a/qobject.h
> > +++ b/qobject.h
> > @@ -43,6 +43,7 @@ typedef enum {
> > QTYPE_QLIST,
> > QTYPE_QFLOAT,
> > QTYPE_QBOOL,
> > + QTYPE_QERROR,
> > } qtype_code;
> >
> > struct QObject;
>
> Erroneous QERRs are detected only when they're passed to
> qerror_from_info() at run-time, i.e. when the error happens. Likewise
> for erroneous qerror_table[].desc. Perhaps a unit test to ensure
> qerror_table[] is sane would make sense. Can't protect from passing
> unknown errors to qerror_from_info(), but that shouldn't be a problem in
> practice.
We could also have a debug function that could run once at startup
and do the check.
- [Qemu-devel] [PATCH 00/10]: QError v4, Luiz Capitulino, 2009/11/17
- [Qemu-devel] [PATCH 01/10] QJSON: Introduce qobject_from_jsonv(), Luiz Capitulino, 2009/11/17
- [Qemu-devel] [PATCH 02/10] QString: Introduce qstring_append_chr(), Luiz Capitulino, 2009/11/17
- [Qemu-devel] [PATCH 03/10] QString: Introduce qstring_append_int(), Luiz Capitulino, 2009/11/17
- [Qemu-devel] [PATCH 04/10] QString: Introduce qstring_from_substr(), Luiz Capitulino, 2009/11/17
- [Qemu-devel] [PATCH 05/10] utests: Add qstring_append_chr() unit-test, Luiz Capitulino, 2009/11/17
- [Qemu-devel] [PATCH 06/10] utests: Add qstring_from_substr() unit-test, Luiz Capitulino, 2009/11/17
- [Qemu-devel] [PATCH 07/10] Introduce QError, Luiz Capitulino, 2009/11/17
- Re: [Qemu-devel] [PATCH 07/10] Introduce QError, Daniel P. Berrange, 2009/11/18
[Qemu-devel] [PATCH 10/10] monitor: do_info_balloon(): use QError, Luiz Capitulino, 2009/11/17
[Qemu-devel] [PATCH 08/10] monitor: QError support, Luiz Capitulino, 2009/11/17