Daniel P. Berrange wrote:
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.
Committed this patch, minus the above hunk. I'll rework it to have the .default as Dan suggested in 3/5. Chris Lalancette -- Chris Lalancette