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
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list