Re: [libvirt] [PATCH 2/5]: Properly set errors on unknown format types

On Tue, Oct 21, 2008 at 03:57:00PM +0200, Chris Lalancette wrote: > Because of my patch last week that converted the various virStorage*FromString > and virStorage*ToString implementations to the generic VIR_ENUM_IMPL, there were > a couple of places that didn't properly set errors when they failed. This patch > fixes these places up. It also adds an additional check in virEnumFromString(), > so that if a NULL type is passed in, it fails instead of SEGV'ing. ACK to the setting of errors, but not.... > diff -up ./src/storage_conf.c.p1 ./src/storage_conf.c > --- ./src/storage_conf.c.p1 2008-10-21 14:48:10.000000000 +0200 > +++ ./src/storage_conf.c 2008-10-21 14:56:59.000000000 +0200 > @@ -276,6 +276,8 @@ virStoragePoolDefParseDoc(virConnectPtr > if (options->formatFromString) { > char *format = virXPathString(conn, "string(/pool/source/format/@type)", ctxt); > if ((ret->source.format = (options->formatFromString)(format)) < 0) { > + virStorageReportError(conn, VIR_ERR_XML_ERROR, > + _("unknown pool format type %s"), format); > VIR_FREE(format); > goto cleanup; > } > @@ -521,8 +526,12 @@ virStoragePoolDefFormat(virConnectPtr co > > if (options->formatToString) { > const char *format = (options->formatToString)(def->source.format); > - if (!format) > + if (!format) { > + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, > + _("unknown pool format number %d"), > + def->source.format); > goto cleanup; > + } > virBufferVSprintf(&buf," <format type='%s'/>\n", format); > } > > @@ -751,6 +760,8 @@ virStorageVolDefParseDoc(virConnectPtr c > if (options->formatFromString) { > char *format = virXPathString(conn, "string(/volume/target/format/@type)", ctxt); > if ((ret->target.format = (options->formatFromString)(format)) < 0) { > + virStorageReportError(conn, VIR_ERR_XML_ERROR, > + _("unknown volume format type %s"), format); > VIR_FREE(format); > goto cleanup; > } > @@ -885,8 +896,12 @@ virStorageVolDefFormat(virConnectPtr con > > if (options->formatToString) { > const char *format = (options->formatToString)(def->target.format); > - if (!format) > + if (!format) { > + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, > + _("unknown volume format number %d"), > + def->target.format); > goto cleanup; > + } > virBufferVSprintf(&buf," <format type='%s'/>\n", format); > } > > diff -up ./src/util.c.p1 ./src/util.c > --- ./src/util.c.p1 2008-10-21 14:59:37.000000000 +0200 > +++ ./src/util.c 2008-10-21 15:00:07.000000000 +0200 > @@ -1018,6 +1018,10 @@ int virEnumFromString(const char *const* > const char *type) > { > unsigned int i; > + > + if (type == NULL) > + return -1; > + > for (i = 0 ; i < ntypes ; i++) > if (STREQ(types[i], type)) > return i; But not to this hunk - its better handled by having an explicit default format for parsing, because we need to have a concept of defaults, to be able to distinguish it from truely unknown settings.