[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] virDomainDiskDefAssignAddress: return int, not void



On Fri, Mar 19, 2010 at 05:00:21PM +0100, Jim Meyering wrote:
> Per discussion a week or so ago,
> here's a fix for virDomainDiskDefAssignAddress.
> However, this change is incomplete, because making that function
> reject erroneous input has exposed problems elsewhere.
> For starters, this change causes three previously passing tests to fail:
> 
> 
>       TEST: virshtest
>             .!.!!!!!!!!!!!!!                         16  FAIL
>       FAIL: virshtest
> 
>       TEST: int-overflow
>       --- err 2010-03-19 16:50:29.869979267 +0100
>       +++ exp 2010-03-19 16:50:29.847854045 +0100
>       @@ -1,2 +1 @@
>       -error: Unknown failure
>       -error: failed to connect to the hypervisor
>       +error: failed to get domain '4294967298'
>       FAIL: int-overflow
> 
>       TEST: xml2sexprtest
>             .................!.....!................ 40
>             ...                                      43  FAIL
>       FAIL: xml2sexprtest
> 
> 
> Those fail because virDomainDiskDefAssignAddress ends
> up processing a "def" with def->dst values of "xvda:disk"
> and "sda1", both of which make virDiskNameToIndex(def->dst) return -1.
> 
> I confirmed that simply removing any ":disk" suffix
> and mapping "sda1" to "sda" would solve the problem,
> but the question is where to make that change.
> 
> There are numerous input XML files that mention "sda1",
> so changing test inputs is probably not an option.
> 
> There is already code to remove the :disk suffix, e.e., here:
> 
>     src/xen/xend_internal.c:  } else if (STREQ (offset, ":disk")) {
>     ...
>     src/xen/xend_internal.c-  offset[0] = '\0';
> 
> Suggestions?

Need to figure out why the virDomainDefPtr  object ends up with
a ':disk' suffix. This should definitely be stripped by the SEXPR
parser before it gets into the virDomainDefPtr object.

The 'sda1' is a valid device (unfortunately). So for that we'll need
to make the virDiskNameToIndex method strip/ignore any trailing
digits.

> 
> 
> >From 3d2b32ad3309ee10ae48f438f61134d2fa00a63a Mon Sep 17 00:00:00 2001
> From: Jim Meyering <meyering redhat com>
> Date: Mon, 15 Mar 2010 21:42:01 +0100
> Subject: [PATCH] virDomainDiskDefAssignAddress: return int, not void
> 
> Before, this function would blindly accept an invalid def->dst
> and then abuse the idx=-1 it would get from virDiskNameToIndex,
> when passing it invalid strings like "xvda:disk" and "sda1".
> Now, this function returns -1 upon failure.
> * src/conf/domain_conf.c (virDomainDiskDefAssignAddress): as above.
> Update callers.
> * src/conf/domain_conf.h: Update prototype.
> * src/qemu/qemu_conf.c: Update callers.
> ---
>  src/conf/domain_conf.c |   11 ++++++++---
>  src/conf/domain_conf.h |    4 ++--
>  src/qemu/qemu_conf.c   |   11 +++++++++--
>  3 files changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 56c5239..5968405 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1233,10 +1233,12 @@ cleanup:
>  }
> 
> 
> -void
> +int
>  virDomainDiskDefAssignAddress(virDomainDiskDefPtr def)
>  {
>      int idx = virDiskNameToIndex(def->dst);
> +    if (idx < 0)
> +        return -1;
> 
>      switch (def->bus) {
>      case VIR_DOMAIN_DISK_BUS_SCSI:
> @@ -1270,6 +1272,8 @@ virDomainDiskDefAssignAddress(virDomainDiskDefPtr def)
>          /* Other disk bus's aren't controller based */
>          break;
>      }
> +
> +    return 0;
>  }
> 
>  /* Parse the XML definition for a disk
> @@ -1498,8 +1502,9 @@ virDomainDiskDefParseXML(xmlNodePtr node,
>      def->serial = serial;
>      serial = NULL;
> 
> -    if (def->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
> -        virDomainDiskDefAssignAddress(def);
> +    if (def->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE
> +        && virDomainDiskDefAssignAddress(def))
> +        goto error;

This should be '&& virDomainDiskDefAssignAddress(def) < 0'


>      VIR_FREE(bus);
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 0540a77..44fff0c 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1,7 +1,7 @@
>  /*
>   * domain_conf.h: domain XML processing
>   *
> - * Copyright (C) 2006-2008 Red Hat, Inc.
> + * Copyright (C) 2006-2008, 2010 Red Hat, Inc.
>   * Copyright (C) 2006-2008 Daniel P. Berrange
>   *
>   * This library is free software; you can redistribute it and/or
> @@ -855,7 +855,7 @@ int virDomainDiskInsert(virDomainDefPtr def,
>                          virDomainDiskDefPtr disk);
>  void virDomainDiskInsertPreAlloced(virDomainDefPtr def,
>                                     virDomainDiskDefPtr disk);
> -void virDomainDiskDefAssignAddress(virDomainDiskDefPtr def);
> +int virDomainDiskDefAssignAddress(virDomainDiskDefPtr def);
> 
>  int virDomainControllerInsert(virDomainDefPtr def,
>                                virDomainControllerDefPtr controller);
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index fb23c52..95d2e47 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -4766,7 +4766,13 @@ qemuParseCommandLineDisk(const char *val,
>      else
>          def->dst[2] = 'a' + idx;
> 
> -    virDomainDiskDefAssignAddress(def);
> +    if (virDomainDiskDefAssignAddress(def) != 0) {
> +        qemuReportError(VIR_ERR_INTERNAL_ERROR,
> +                        _("invalid device name '%s'"), def->dst);
> +        virDomainDiskDefFree(def);
> +        def = NULL;
> +        /* fall through to "cleanup" */
> +    }

I prefer it that we use  'XXXX < 0' for error checks, since we sometimes
use positive values for non-error scenarios.

> 
>  cleanup:
>      for (i = 0 ; i < nkeywords ; i++) {
> @@ -5574,7 +5580,8 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
>                  goto no_memory;
>              }
> 
> -            virDomainDiskDefAssignAddress(disk);
> +            if (virDomainDiskDefAssignAddress(disk) != 0)
> +                goto error;

Likewise here.


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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]