From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:45754) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SuPZw-00081U-Kp for qemu-devel@nongnu.org; Thu, 26 Jul 2012 11:02:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SuPZm-0004cB-Kt for qemu-devel@nongnu.org; Thu, 26 Jul 2012 11:02:16 -0400 Received: from mx1.redhat.com ([209.132.183.28]:3512) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SuPZm-0004c0-CV for qemu-devel@nongnu.org; Thu, 26 Jul 2012 11:02:06 -0400 Date: Thu, 26 Jul 2012 12:02:41 -0300 From: Luiz Capitulino Message-ID: <20120726120241.01f7d965@doriath.home> In-Reply-To: <87txwuaei4.fsf@blackfin.pond.sub.org> References: <1343249431-9245-1-git-send-email-lcapitulino@redhat.com> <1343249431-9245-4-git-send-email-lcapitulino@redhat.com> <87txwuaei4.fsf@blackfin.pond.sub.org> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 03/14] qerror: drop qerror_abort() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: peter.maydell@linaro.org, pbonzini@redhat.com, aliguori@us.ibm.com, qemu-devel@nongnu.org, afaerber@suse.de On Thu, 26 Jul 2012 14:33:23 +0200 Markus Armbruster wrote: > Luiz Capitulino writes: > > > Previously, developers had to create error dicts manually and also enter > > the matching desc table entry. Thus, qerro_abort() was added to help > > s/qerro_/qerror_/ > > > catching bad error dicts. > > > > Today, all that stuff is generated automatically which makes the chance > > of a bad generated dict quite low. > > The generator can only generate well-formed dicts, right? I didn't say that. I said the chances of generating a bad dict is low, specially in production. > > > Also, qerror_abort() would... _abort_ on a bad error dict! > > In my opinion, it's perfectly fine to abort on a programming error. In this case it's quite easy to recover, so why should we abort? > > > This commit maintains the same verification logic on the error object > > but drops the abort() in favor of reporting UndefinedError for bad dicts > > (and also print a debug message to stderr). > > I certainly won't object to your elaborate handling, but me, I'd simply > replace qerror_abort() by straight abort() and be done with it *shrug* Well, honestly speaking, I did this while I was re-reading that code and I ended up dropping it down this series (remember this is an RFC) :) But I think I'll add it back for v1. > > > Signed-off-by: Luiz Capitulino > > --- > > qerror.c | 83 +++++++++++++++++++++++++++++++++++----------------------------- > > 1 file changed, 46 insertions(+), 37 deletions(-) > > > > diff --git a/qerror.c b/qerror.c > > index d428b52..74b3659 100644 > > --- a/qerror.c > > +++ b/qerror.c > > @@ -38,55 +38,50 @@ static QError *qerror_new(void) > > return qerr; > > } > > > > -static void GCC_FMT_ATTR(2, 3) qerror_abort(const QError *qerr, > > - const char *fmt, ...) > > -{ > > - va_list ap; > > - > > - fprintf(stderr, "qerror: bad call in function '%s':\n", qerr->func); > > - fprintf(stderr, "qerror: -> "); > > - > > - va_start(ap, fmt); > > - vfprintf(stderr, fmt, ap); > > - va_end(ap); > > - > > - fprintf(stderr, "\nqerror: call at %s:%d\n", qerr->file, qerr->linenr); > > - abort(); > > -} > > - > > -static void GCC_FMT_ATTR(2, 0) qerror_set_data(QError *qerr, > > - const char *fmt, va_list *va) > > +static QDict *error_object_from_fmt(const char *fmt, va_list *va) > > { > > QObject *obj; > > + QDict *ret; > > > > obj = qobject_from_jsonv(fmt, va); > > if (!obj) { > > - qerror_abort(qerr, "invalid format '%s'", fmt); > > + fprintf(stderr, "invalid json in error dict '%s'\n", fmt); > > + return NULL; > > } > > if (qobject_type(obj) != QTYPE_QDICT) { > > - qerror_abort(qerr, "error format is not a QDict '%s'", fmt); > > + fprintf(stderr, "error is not a dict '%s'\n", fmt); > > + goto out_free; > > } > > > > - qerr->error = qobject_to_qdict(obj); > > - > > - obj = qdict_get(qerr->error, "class"); > > + ret = qobject_to_qdict(obj); > > + obj = qdict_get(ret, "class"); > > if (!obj) { > > - qerror_abort(qerr, "missing 'class' key in '%s'", fmt); > > + fprintf(stderr, "missing 'class' key in '%s'\n", fmt); > > + goto out_free; > > } > > if (qobject_type(obj) != QTYPE_QSTRING) { > > - qerror_abort(qerr, "'class' key value should be a QString"); > > + fprintf(stderr, "'class' key value should be a string in '%s'\n", fmt); > > + goto out_free; > > } > > - > > - obj = qdict_get(qerr->error, "data"); > > + > > + obj = qdict_get(ret, "data"); > > if (!obj) { > > - qerror_abort(qerr, "missing 'data' key in '%s'", fmt); > > + fprintf(stderr, "missing 'data' key in '%s'\n", fmt); > > + goto out_free; > > } > > if (qobject_type(obj) != QTYPE_QDICT) { > > - qerror_abort(qerr, "'data' key value should be a QDICT"); > > + fprintf(stderr, "'data' key value should be a dict in '%s'\n", fmt); > > + goto out_free; > > } > > + > > + return ret; > > + > > +out_free: > > + qobject_decref(obj); > > + return NULL; > > } > > > > -static void qerror_set_desc(QError *qerr, const char *fmt) > > +static const QErrorStringTable *error_get_desc(const char *fmt) > > { > > int i; > > > > @@ -94,12 +89,17 @@ static void qerror_set_desc(QError *qerr, const char *fmt) > > > > for (i = 0; qerror_table[i].error_fmt; i++) { > > if (strcmp(qerror_table[i].error_fmt, fmt) == 0) { > > - qerr->entry = &qerror_table[i]; > > - return; > > + return &qerror_table[i]; > > } > > } > > > > - qerror_abort(qerr, "error format '%s' not found", fmt); > > + fprintf(stderr, "error format '%s' not found\n", fmt); > > + return NULL; > > +} > > + > > +static QDict *build_error_no_arg(const char *fmt) > > +{ > > + return qobject_to_qdict(qobject_from_jsonv(fmt, NULL)); > > } > > > > /** > > @@ -127,13 +127,22 @@ static QError *qerror_from_info(const char *file, int linenr, const char *func, > > qerr->file = file; > > qerr->func = func; > > > > - if (!fmt) { > > - qerror_abort(qerr, "QDict not specified"); > > + qerr->error = error_object_from_fmt(fmt, va); > > + if (!qerr->error) { > > + goto bad_err; > > + } > > + > > + qerr->entry = error_get_desc(fmt); > > + if (!qerr->entry) { > > + QDECREF(qerr->error); > > + goto bad_err; > > } > > > > - qerror_set_data(qerr, fmt, va); > > - qerror_set_desc(qerr, fmt); > > + return qerr; > > > > +bad_err: > > + qerr->error = build_error_no_arg(QERR_UNDEFINED_ERROR); > > Nit: defeats gcc's format checking. This shouldn't: > > qerr->error = qobject_to_qdict(qobject_from_json(QERR_UNDEFINED_ERROR)); > > > + qerr->entry = error_get_desc(QERR_UNDEFINED_ERROR); > > return qerr; > > } >