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

[Daniel P. Berrange]

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.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|