| Message ID | 1343249431-9245-4-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | New |
| Headers | show |
Luiz Capitulino <lcapitulino@redhat.com> 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? > Also, qerror_abort() would... _abort_ on a bad error dict! In my opinion, it's perfectly fine to abort on a programming error. > 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* > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > --- > 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; > }
On 07/25/2012 02:50 PM, Luiz Capitulino wrote: > 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. > > Also, qerror_abort() would... _abort_ on a bad error dict! > > 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). > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > ---
On Thu, 26 Jul 2012 14:33:23 +0200 Markus Armbruster <armbru@redhat.com> wrote: > Luiz Capitulino <lcapitulino@redhat.com> 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 <lcapitulino@redhat.com> > > --- > > 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; > > } >
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); + qerr->entry = error_get_desc(QERR_UNDEFINED_ERROR); return qerr; }
Previously, developers had to create error dicts manually and also enter the matching desc table entry. Thus, qerro_abort() was added to help catching bad error dicts. Today, all that stuff is generated automatically which makes the chance of a bad generated dict quite low. Also, qerror_abort() would... _abort_ on a bad error dict! 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). Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> --- qerror.c | 83 +++++++++++++++++++++++++++++++++++----------------------------- 1 file changed, 46 insertions(+), 37 deletions(-)