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

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?

>From 3d2b32ad3309ee10ae48f438f61134d2fa00a63a Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com&gt; 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;

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" */ + }

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;