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