[libvirt] [PATCH 0/3] Misc fixes to storage pool & network drivers
These three patches fix misc problems in the storage pool and network drivers identified by the TCK Daniel
The storage pool driver is not doing correct checking for duplicate UUID/name values. This introduces a new method virStoragePoolObjIsDuplicate, based on the previously written virDomainObjIsDuplicate. * src/conf/storage_conf.c, src/conf/storage_conf.c, src/libvirt_private.syms: Add virStoragePoolObjIsDuplicate, * src/storage/storage_driver.c: Call virStoragePoolObjIsDuplicate for checking uniqueness of uuid/names --- src/conf/storage_conf.c | 63 ++++++++++++++++++++++++++++++++++++++++++ src/conf/storage_conf.h | 4 ++ src/libvirt_private.syms | 1 + src/storage/storage_driver.c | 14 ++------ 4 files changed, 72 insertions(+), 10 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 422e76a..9ae1a89 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1628,6 +1628,69 @@ char *virStoragePoolSourceListFormat(virStoragePoolSourceListPtr def) } +/* + * virStoragePoolObjIsDuplicate: + * @doms : virStoragePoolObjListPtr to search + * @def : virStoragePoolDefPtr definition of pool to lookup + * @check_active: If true, ensure that pool is not active + * + * Returns: -1 on error + * 0 if pool is new + * 1 if pool is a duplicate + */ +int virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools, + virStoragePoolDefPtr def, + unsigned int check_active) +{ + int ret = -1; + int dupPool = 0; + virStoragePoolObjPtr pool = NULL; + + /* See if a Pool with matching UUID already exists */ + pool = virStoragePoolObjFindByUUID(pools, def->uuid); + if (pool) { + /* UUID matches, but if names don't match, refuse it */ + if (STRNEQ(pool->def->name, def->name)) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(pool->def->uuid, uuidstr); + virStorageReportError(VIR_ERR_OPERATION_FAILED, + _("pool '%s' is already defined with uuid %s"), + pool->def->name, uuidstr); + goto cleanup; + } + + if (check_active) { + /* UUID & name match, but if Pool is already active, refuse it */ + if (virStoragePoolObjIsActive(pool)) { + virStorageReportError(VIR_ERR_OPERATION_INVALID, + _("pool is already active as '%s'"), + pool->def->name); + goto cleanup; + } + } + + dupPool = 1; + } else { + /* UUID does not match, but if a name matches, refuse it */ + pool = virStoragePoolObjFindByName(pools, def->name); + if (pool) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(pool->def->uuid, uuidstr); + virStorageReportError(VIR_ERR_OPERATION_FAILED, + _("pool '%s' already exists with uuid %s"), + def->name, uuidstr); + goto cleanup; + } + } + + ret = dupPool; +cleanup: + if (pool) + virStoragePoolObjUnlock(pool); + return ret; +} + + void virStoragePoolObjLock(virStoragePoolObjPtr obj) { virMutexLock(&obj->lock); diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 5813489..bedee9e 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -379,6 +379,10 @@ virStoragePoolSourcePtr virStoragePoolSourceListNewSource(virStoragePoolSourceListPtr list); char *virStoragePoolSourceListFormat(virStoragePoolSourceListPtr def); +int virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools, + virStoragePoolDefPtr def, + unsigned int check_active); + void virStoragePoolObjLock(virStoragePoolObjPtr obj); void virStoragePoolObjUnlock(virStoragePoolObjPtr obj); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6cb3d66..e7cd6dd 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -610,6 +610,7 @@ virStoragePoolTypeFromString; virStoragePartedFsTypeTypeToString; virStoragePoolObjLock; virStoragePoolObjUnlock; +virStoragePoolObjIsDuplicate; # storage_encryption_conf.h virStorageEncryptionFree; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 0870f74..e53317a 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -521,17 +521,8 @@ storagePoolCreate(virConnectPtr conn, if (!(def = virStoragePoolDefParseString(xml))) goto cleanup; - pool = virStoragePoolObjFindByUUID(&driver->pools, def->uuid); - if (!pool) - pool = virStoragePoolObjFindByName(&driver->pools, def->name); - - if (pool) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("storage pool already exists")); - virStoragePoolObjUnlock(pool); - pool = NULL; + if (virStoragePoolObjIsDuplicate(&driver->pools, def, 1) < 0) goto cleanup; - } if ((backend = virStorageBackendForType(def->type)) == NULL) goto cleanup; @@ -579,6 +570,9 @@ storagePoolDefine(virConnectPtr conn, if (!(def = virStoragePoolDefParseString(xml))) goto cleanup; + if (virStoragePoolObjIsDuplicate(&driver->pools, def, 0) < 0) + goto cleanup; + if (virStorageBackendForType(def->type) == NULL) goto cleanup; -- 1.6.6.1
On 05/27/2010 12:00 PM, Daniel P. Berrange wrote:
The storage pool driver is not doing correct checking for duplicate UUID/name values. This introduces a new method virStoragePoolObjIsDuplicate, based on the previously written virDomainObjIsDuplicate.
* src/conf/storage_conf.c, src/conf/storage_conf.c, src/libvirt_private.syms: Add virStoragePoolObjIsDuplicate, * src/storage/storage_driver.c: Call virStoragePoolObjIsDuplicate for checking uniqueness of uuid/names --- src/conf/storage_conf.c | 63 ++++++++++++++++++++++++++++++++++++++++++ src/conf/storage_conf.h | 4 ++ src/libvirt_private.syms | 1 + src/storage/storage_driver.c | 14 ++------ 4 files changed, 72 insertions(+), 10 deletions(-)
ACK - Cole
The storage pool driver is mistakenly using the error code VIR_ERR_INVALID_STORAGE_POOL which is for diagnosing invalid pointers. This patch switches it to use VIR_ERR_NO_STORAGE_POOL which is the correct code for cases where the storage pool does not exist * src/storage/storage_driver.c: Replace VIR_ERR_INVALID_STORAGE_POOL with VIR_ERR_NO_STORAGE_POOL --- src/storage/storage_driver.c | 50 +++++++++++++++++++++--------------------- 1 files changed, 25 insertions(+), 25 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index e53317a..4ebbced 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -605,7 +605,7 @@ storagePoolUndefine(virStoragePoolPtr obj) { storageDriverLock(driver); pool = virStoragePoolObjFindByUUID(&driver->pools, obj->uuid); if (!pool) { - virStorageReportError(VIR_ERR_INVALID_STORAGE_POOL, + virStorageReportError(VIR_ERR_NO_STORAGE_POOL, "%s", _("no storage pool with matching uuid")); goto cleanup; } @@ -659,7 +659,7 @@ storagePoolStart(virStoragePoolPtr obj, storageDriverUnlock(driver); if (!pool) { - virStorageReportError(VIR_ERR_INVALID_STORAGE_POOL, + virStorageReportError(VIR_ERR_NO_STORAGE_POOL, "%s", _("no storage pool with matching uuid")); goto cleanup; } @@ -704,7 +704,7 @@ storagePoolBuild(virStoragePoolPtr obj, storageDriverUnlock(driver); if (!pool) { - virStorageReportError(VIR_ERR_INVALID_STORAGE_POOL, + virStorageReportError(VIR_ERR_NO_STORAGE_POOL, "%s", _("no storage pool with matching uuid")); goto cleanup; } @@ -741,7 +741,7 @@ storagePoolDestroy(virStoragePoolPtr obj) { pool = virStoragePoolObjFindByUUID(&driver->pools, obj->uuid); if (!pool) { - virStorageReportError(VIR_ERR_INVALID_STORAGE_POOL, + virStorageReportError(VIR_ERR_NO_STORAGE_POOL, "%s", _("no storage pool with matching uuid")); goto cleanup; } @@ -797,7 +797,7 @@ storagePoolDelete(virStoragePoolPtr obj, storageDriverUnlock(driver); if (!pool) { - virStorageReportError(VIR_ERR_INVALID_STORAGE_POOL, + virStorageReportError(VIR_ERR_NO_STORAGE_POOL, "%s", _("no storage pool with matching uuid")); goto cleanup; } @@ -846,7 +846,7 @@ storagePoolRefresh(virStoragePoolPtr obj, pool = virStoragePoolObjFindByUUID(&driver->pools, obj->uuid); if (!pool) { - virStorageReportError(VIR_ERR_INVALID_STORAGE_POOL, + virStorageReportError(VIR_ERR_NO_STORAGE_POOL, "%s", _("no storage pool with matching uuid")); goto cleanup; } @@ -902,7 +902,7 @@ storagePoolGetInfo(virStoragePoolPtr obj, storageDriverUnlock(driver); if (!pool) { - virStorageReportError(VIR_ERR_INVALID_STORAGE_POOL, + virStorageReportError(VIR_ERR_NO_STORAGE_POOL, "%s", _("no storage pool with matching uuid")); goto cleanup; } @@ -938,7 +938,7 @@ storagePoolDumpXML(virStoragePoolPtr obj, storageDriverUnlock(driver); if (!pool) { - virStorageReportError(VIR_ERR_INVALID_STORAGE_POOL, + virStorageReportError(VIR_ERR_NO_STORAGE_POOL, "%s", _("no storage pool with matching uuid")); goto cleanup; } @@ -963,7 +963,7 @@ storagePoolGetAutostart(virStoragePoolPtr obj, storageDriverUnlock(driver); if (!pool) { - virStorageReportError(VIR_ERR_INVALID_STORAGE_POOL, + virStorageReportError(VIR_ERR_NO_STORAGE_POOL, "%s", _("no pool with matching uuid")); goto cleanup; } @@ -992,13 +992,13 @@ storagePoolSetAutostart(virStoragePoolPtr obj, pool = virStoragePoolObjFindByUUID(&driver->pools, obj->uuid); if (!pool) { - virStorageReportError(VIR_ERR_INVALID_STORAGE_POOL, + virStorageReportError(VIR_ERR_NO_STORAGE_POOL, "%s", _("no pool with matching uuid")); goto cleanup; } if (!pool->configFile) { - virStorageReportError(VIR_ERR_INVALID_ARG, + virStorageReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("pool has no config file")); goto cleanup; } @@ -1054,7 +1054,7 @@ storagePoolNumVolumes(virStoragePoolPtr obj) { storageDriverUnlock(driver); if (!pool) { - virStorageReportError(VIR_ERR_INVALID_STORAGE_POOL, + virStorageReportError(VIR_ERR_NO_STORAGE_POOL, "%s", _("no storage pool with matching uuid")); goto cleanup; } @@ -1087,7 +1087,7 @@ storagePoolListVolumes(virStoragePoolPtr obj, storageDriverUnlock(driver); if (!pool) { - virStorageReportError(VIR_ERR_INVALID_STORAGE_POOL, + virStorageReportError(VIR_ERR_NO_STORAGE_POOL, "%s", _("no storage pool with matching uuid")); goto cleanup; } @@ -1132,7 +1132,7 @@ storageVolumeLookupByName(virStoragePoolPtr obj, storageDriverUnlock(driver); if (!pool) { - virStorageReportError(VIR_ERR_INVALID_STORAGE_POOL, + virStorageReportError(VIR_ERR_NO_STORAGE_POOL, "%s", _("no storage pool with matching uuid")); goto cleanup; } @@ -1186,7 +1186,7 @@ storageVolumeLookupByKey(virConnectPtr conn, storageDriverUnlock(driver); if (!ret) - virStorageReportError(VIR_ERR_INVALID_STORAGE_VOL, + virStorageReportError(VIR_ERR_NO_STORAGE_VOL, "%s", _("no storage vol with matching key")); return ret; @@ -1237,7 +1237,7 @@ storageVolumeLookupByPath(virConnectPtr conn, } if (!ret) - virStorageReportError(VIR_ERR_INVALID_STORAGE_VOL, + virStorageReportError(VIR_ERR_NO_STORAGE_VOL, "%s", _("no storage vol with matching path")); cleanup: @@ -1263,7 +1263,7 @@ storageVolumeCreateXML(virStoragePoolPtr obj, storageDriverUnlock(driver); if (!pool) { - virStorageReportError(VIR_ERR_INVALID_STORAGE_POOL, + virStorageReportError(VIR_ERR_NO_STORAGE_POOL, "%s", _("no storage pool with matching uuid")); goto cleanup; } @@ -1282,7 +1282,7 @@ storageVolumeCreateXML(virStoragePoolPtr obj, goto cleanup; if (virStorageVolDefFindByName(pool, voldef->name)) { - virStorageReportError(VIR_ERR_INVALID_STORAGE_POOL, + virStorageReportError(VIR_ERR_NO_STORAGE_VOL, "%s", _("storage vol already exists")); goto cleanup; } @@ -1384,7 +1384,7 @@ storageVolumeCreateXMLFrom(virStoragePoolPtr obj, } storageDriverUnlock(driver); if (!pool) { - virStorageReportError(VIR_ERR_INVALID_STORAGE_POOL, + virStorageReportError(VIR_ERR_NO_STORAGE_POOL, "%s", _("no storage pool with matching uuid")); goto cleanup; } @@ -1424,7 +1424,7 @@ storageVolumeCreateXMLFrom(virStoragePoolPtr obj, goto cleanup; if (virStorageVolDefFindByName(pool, newvol->name)) { - virStorageReportError(VIR_ERR_INVALID_STORAGE_POOL, + virStorageReportError(VIR_ERR_INTERNAL_ERROR, _("storage volume name '%s' already in use."), newvol->name); goto cleanup; @@ -1688,7 +1688,7 @@ storageVolumeWipe(virStorageVolPtr obj, storageDriverUnlock(driver); if (!pool) { - virStorageReportError(VIR_ERR_INVALID_STORAGE_POOL, + virStorageReportError(VIR_ERR_NO_STORAGE_POOL, "%s", _("no storage pool with matching uuid")); goto out; } @@ -1745,7 +1745,7 @@ storageVolumeDelete(virStorageVolPtr obj, storageDriverUnlock(driver); if (!pool) { - virStorageReportError(VIR_ERR_INVALID_STORAGE_POOL, + virStorageReportError(VIR_ERR_NO_STORAGE_POOL, "%s", _("no storage pool with matching uuid")); goto cleanup; } @@ -1824,7 +1824,7 @@ storageVolumeGetInfo(virStorageVolPtr obj, storageDriverUnlock(driver); if (!pool) { - virStorageReportError(VIR_ERR_INVALID_STORAGE_POOL, + virStorageReportError(VIR_ERR_NO_STORAGE_POOL, "%s", _("no storage pool with matching uuid")); goto cleanup; } @@ -1877,7 +1877,7 @@ storageVolumeGetXMLDesc(virStorageVolPtr obj, storageDriverUnlock(driver); if (!pool) { - virStorageReportError(VIR_ERR_INVALID_STORAGE_POOL, + virStorageReportError(VIR_ERR_NO_STORAGE_POOL, "%s", _("no storage pool with matching uuid")); goto cleanup; } @@ -1924,7 +1924,7 @@ storageVolumeGetPath(virStorageVolPtr obj) { pool = virStoragePoolObjFindByName(&driver->pools, obj->pool); storageDriverUnlock(driver); if (!pool) { - virStorageReportError(VIR_ERR_INVALID_STORAGE_POOL, + virStorageReportError(VIR_ERR_NO_STORAGE_POOL, "%s", _("no storage pool with matching uuid")); goto cleanup; } -- 1.6.6.1
On 05/27/2010 12:00 PM, Daniel P. Berrange wrote:
The storage pool driver is mistakenly using the error code VIR_ERR_INVALID_STORAGE_POOL which is for diagnosing invalid pointers. This patch switches it to use VIR_ERR_NO_STORAGE_POOL which is the correct code for cases where the storage pool does not exist
* src/storage/storage_driver.c: Replace VIR_ERR_INVALID_STORAGE_POOL with VIR_ERR_NO_STORAGE_POOL
ACK - Cole
On Thu, May 27, 2010 at 05:00:11PM +0100, Daniel P. Berrange wrote:
The storage pool driver is mistakenly using the error code VIR_ERR_INVALID_STORAGE_POOL which is for diagnosing invalid pointers. This patch switches it to use VIR_ERR_NO_STORAGE_POOL which is the correct code for cases where the storage pool does not exist
* src/storage/storage_driver.c: Replace VIR_ERR_INVALID_STORAGE_POOL with VIR_ERR_NO_STORAGE_POOL --- src/storage/storage_driver.c | 50 +++++++++++++++++++++--------------------- 1 files changed, 25 insertions(+), 25 deletions(-)
ACK Dave
The network driver is not doing correct checking for duplicate UUID/name values. This introduces a new method virNetworkObjIsDuplicate, based on the previously written virDomainObjIsDuplicate. * src/conf/network_conf.c, src/conf/network_conf.c, src/libvirt_private.syms: Add virNetworkObjIsDuplicate, * src/network/bridge_driver.c: Call virNetworkObjIsDuplicate for checking uniqueness of uuid/names --- src/conf/network_conf.c | 65 +++++++++++++++++++++++++++++++++++++++++++ src/conf/network_conf.h | 4 ++ src/libvirt_private.syms | 1 + src/network/bridge_driver.c | 7 ++++ 4 files changed, 77 insertions(+), 0 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 06537d1..347fc0b 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -940,6 +940,71 @@ error: return ret; } + +/* + * virNetworkObjIsDuplicate: + * @doms : virNetworkObjListPtr to search + * @def : virNetworkDefPtr definition of network to lookup + * @check_active: If true, ensure that network is not active + * + * Returns: -1 on error + * 0 if network is new + * 1 if network is a duplicate + */ +int +virNetworkObjIsDuplicate(virNetworkObjListPtr doms, + virNetworkDefPtr def, + unsigned int check_active) +{ + int ret = -1; + int dupVM = 0; + virNetworkObjPtr vm = NULL; + + /* See if a VM with matching UUID already exists */ + vm = virNetworkFindByUUID(doms, def->uuid); + if (vm) { + /* UUID matches, but if names don't match, refuse it */ + if (STRNEQ(vm->def->name, def->name)) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(vm->def->uuid, uuidstr); + virNetworkReportError(VIR_ERR_OPERATION_FAILED, + _("network '%s' is already defined with uuid %s"), + vm->def->name, uuidstr); + goto cleanup; + } + + if (check_active) { + /* UUID & name match, but if VM is already active, refuse it */ + if (virNetworkObjIsActive(vm)) { + virNetworkReportError(VIR_ERR_OPERATION_INVALID, + _("network is already active as '%s'"), + vm->def->name); + goto cleanup; + } + } + + dupVM = 1; + } else { + /* UUID does not match, but if a name matches, refuse it */ + vm = virNetworkFindByName(doms, def->name); + if (vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(vm->def->uuid, uuidstr); + virNetworkReportError(VIR_ERR_OPERATION_FAILED, + _("network '%s' already exists with uuid %s"), + def->name, uuidstr); + goto cleanup; + } + } + + ret = dupVM; +cleanup: + if (vm) + virNetworkObjUnlock(vm); + return ret; +} + + void virNetworkObjLock(virNetworkObjPtr obj) { virMutexLock(&obj->lock); diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 127a23a..c4956b6 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -169,6 +169,10 @@ int virNetworkSetBridgeName(const virNetworkObjListPtr nets, virNetworkDefPtr def, int check_collision); +int virNetworkObjIsDuplicate(virNetworkObjListPtr doms, + virNetworkDefPtr def, + unsigned int check_active); + void virNetworkObjLock(virNetworkObjPtr obj); void virNetworkObjUnlock(virNetworkObjPtr obj); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e7cd6dd..414e937 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -469,6 +469,7 @@ virNetworkSaveConfig; virNetworkSetBridgeName; virNetworkObjLock; virNetworkObjUnlock; +virNetworkObjIsDuplicate; # nodeinfo.h diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 5d7ef19..01be425 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1265,6 +1265,9 @@ static virNetworkPtr networkCreate(virConnectPtr conn, const char *xml) { if (!(def = virNetworkDefParseString(xml))) goto cleanup; + if (virNetworkObjIsDuplicate(&driver->networks, def, 1) < 0) + goto cleanup; + if (virNetworkSetBridgeName(&driver->networks, def, 1)) goto cleanup; @@ -1295,12 +1298,16 @@ static virNetworkPtr networkDefine(virConnectPtr conn, const char *xml) { virNetworkDefPtr def; virNetworkObjPtr network = NULL; virNetworkPtr ret = NULL; + int dupNet; networkDriverLock(driver); if (!(def = virNetworkDefParseString(xml))) goto cleanup; + if ((dupNet = virNetworkObjIsDuplicate(&driver->networks, def, 0)) < 0) + goto cleanup; + if (virNetworkSetBridgeName(&driver->networks, def, 1)) goto cleanup; -- 1.6.6.1
On 05/27/2010 12:00 PM, Daniel P. Berrange wrote:
The network driver is not doing correct checking for duplicate UUID/name values. This introduces a new method virNetworkObjIsDuplicate, based on the previously written virDomainObjIsDuplicate.
* src/conf/network_conf.c, src/conf/network_conf.c, src/libvirt_private.syms: Add virNetworkObjIsDuplicate, * src/network/bridge_driver.c: Call virNetworkObjIsDuplicate for checking uniqueness of uuid/names --- src/conf/network_conf.c | 65 +++++++++++++++++++++++++++++++++++++++++++ src/conf/network_conf.h | 4 ++ src/libvirt_private.syms | 1 + src/network/bridge_driver.c | 7 ++++ 4 files changed, 77 insertions(+), 0 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 06537d1..347fc0b 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -940,6 +940,71 @@ error: return ret; }
+ +/* + * virNetworkObjIsDuplicate: + * @doms : virNetworkObjListPtr to search + * @def : virNetworkDefPtr definition of network to lookup + * @check_active: If true, ensure that network is not active + * + * Returns: -1 on error + * 0 if network is new + * 1 if network is a duplicate + */ +int +virNetworkObjIsDuplicate(virNetworkObjListPtr doms, + virNetworkDefPtr def, + unsigned int check_active) +{ + int ret = -1; + int dupVM = 0; + virNetworkObjPtr vm = NULL; + + /* See if a VM with matching UUID already exists */ + vm = virNetworkFindByUUID(doms, def->uuid); + if (vm) { + /* UUID matches, but if names don't match, refuse it */ + if (STRNEQ(vm->def->name, def->name)) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(vm->def->uuid, uuidstr); + virNetworkReportError(VIR_ERR_OPERATION_FAILED, + _("network '%s' is already defined with uuid %s"), + vm->def->name, uuidstr); + goto cleanup; + } + + if (check_active) { + /* UUID & name match, but if VM is already active, refuse it */ + if (virNetworkObjIsActive(vm)) { + virNetworkReportError(VIR_ERR_OPERATION_INVALID, + _("network is already active as '%s'"), + vm->def->name); + goto cleanup; + } + } + + dupVM = 1; + } else { + /* UUID does not match, but if a name matches, refuse it */ + vm = virNetworkFindByName(doms, def->name); + if (vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(vm->def->uuid, uuidstr); + virNetworkReportError(VIR_ERR_OPERATION_FAILED, + _("network '%s' already exists with uuid %s"), + def->name, uuidstr); + goto cleanup; + } + } + + ret = dupVM; +cleanup: + if (vm) + virNetworkObjUnlock(vm); + return ret; +} + + void virNetworkObjLock(virNetworkObjPtr obj) { virMutexLock(&obj->lock); diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 127a23a..c4956b6 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -169,6 +169,10 @@ int virNetworkSetBridgeName(const virNetworkObjListPtr nets, virNetworkDefPtr def, int check_collision);
+int virNetworkObjIsDuplicate(virNetworkObjListPtr doms, + virNetworkDefPtr def, + unsigned int check_active); + void virNetworkObjLock(virNetworkObjPtr obj); void virNetworkObjUnlock(virNetworkObjPtr obj);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e7cd6dd..414e937 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -469,6 +469,7 @@ virNetworkSaveConfig; virNetworkSetBridgeName; virNetworkObjLock; virNetworkObjUnlock; +virNetworkObjIsDuplicate;
# nodeinfo.h diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 5d7ef19..01be425 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1265,6 +1265,9 @@ static virNetworkPtr networkCreate(virConnectPtr conn, const char *xml) { if (!(def = virNetworkDefParseString(xml))) goto cleanup;
+ if (virNetworkObjIsDuplicate(&driver->networks, def, 1) < 0) + goto cleanup; + if (virNetworkSetBridgeName(&driver->networks, def, 1)) goto cleanup;
@@ -1295,12 +1298,16 @@ static virNetworkPtr networkDefine(virConnectPtr conn, const char *xml) { virNetworkDefPtr def; virNetworkObjPtr network = NULL; virNetworkPtr ret = NULL; + int dupNet;
Why not just check the error code directly like in the other use? Aside from that, ACK - Cole
networkDriverLock(driver);
if (!(def = virNetworkDefParseString(xml))) goto cleanup;
+ if ((dupNet = virNetworkObjIsDuplicate(&driver->networks, def, 0)) < 0) + goto cleanup; + if (virNetworkSetBridgeName(&driver->networks, def, 1)) goto cleanup;
On Thu, May 27, 2010 at 02:46:33PM -0400, Cole Robinson wrote:
On 05/27/2010 12:00 PM, Daniel P. Berrange wrote:
The network driver is not doing correct checking for duplicate UUID/name values. This introduces a new method virNetworkObjIsDuplicate, based on the previously written virDomainObjIsDuplicate.
* src/conf/network_conf.c, src/conf/network_conf.c, src/libvirt_private.syms: Add virNetworkObjIsDuplicate, * src/network/bridge_driver.c: Call virNetworkObjIsDuplicate for checking uniqueness of uuid/names --- src/conf/network_conf.c | 65 +++++++++++++++++++++++++++++++++++++++++++ src/conf/network_conf.h | 4 ++ src/libvirt_private.syms | 1 + src/network/bridge_driver.c | 7 ++++ 4 files changed, 77 insertions(+), 0 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 06537d1..347fc0b 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -940,6 +940,71 @@ error: return ret; }
+ +/* + * virNetworkObjIsDuplicate: + * @doms : virNetworkObjListPtr to search + * @def : virNetworkDefPtr definition of network to lookup + * @check_active: If true, ensure that network is not active + * + * Returns: -1 on error + * 0 if network is new + * 1 if network is a duplicate + */ +int +virNetworkObjIsDuplicate(virNetworkObjListPtr doms, + virNetworkDefPtr def, + unsigned int check_active) +{ + int ret = -1; + int dupVM = 0; + virNetworkObjPtr vm = NULL; + + /* See if a VM with matching UUID already exists */ + vm = virNetworkFindByUUID(doms, def->uuid); + if (vm) { + /* UUID matches, but if names don't match, refuse it */ + if (STRNEQ(vm->def->name, def->name)) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(vm->def->uuid, uuidstr); + virNetworkReportError(VIR_ERR_OPERATION_FAILED, + _("network '%s' is already defined with uuid %s"), + vm->def->name, uuidstr); + goto cleanup; + } + + if (check_active) { + /* UUID & name match, but if VM is already active, refuse it */ + if (virNetworkObjIsActive(vm)) { + virNetworkReportError(VIR_ERR_OPERATION_INVALID, + _("network is already active as '%s'"), + vm->def->name); + goto cleanup; + } + } + + dupVM = 1; + } else { + /* UUID does not match, but if a name matches, refuse it */ + vm = virNetworkFindByName(doms, def->name); + if (vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(vm->def->uuid, uuidstr); + virNetworkReportError(VIR_ERR_OPERATION_FAILED, + _("network '%s' already exists with uuid %s"), + def->name, uuidstr); + goto cleanup; + } + } + + ret = dupVM; +cleanup: + if (vm) + virNetworkObjUnlock(vm); + return ret; +} + + void virNetworkObjLock(virNetworkObjPtr obj) { virMutexLock(&obj->lock); diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 127a23a..c4956b6 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -169,6 +169,10 @@ int virNetworkSetBridgeName(const virNetworkObjListPtr nets, virNetworkDefPtr def, int check_collision);
+int virNetworkObjIsDuplicate(virNetworkObjListPtr doms, + virNetworkDefPtr def, + unsigned int check_active); + void virNetworkObjLock(virNetworkObjPtr obj); void virNetworkObjUnlock(virNetworkObjPtr obj);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e7cd6dd..414e937 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -469,6 +469,7 @@ virNetworkSaveConfig; virNetworkSetBridgeName; virNetworkObjLock; virNetworkObjUnlock; +virNetworkObjIsDuplicate;
# nodeinfo.h diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 5d7ef19..01be425 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1265,6 +1265,9 @@ static virNetworkPtr networkCreate(virConnectPtr conn, const char *xml) { if (!(def = virNetworkDefParseString(xml))) goto cleanup;
+ if (virNetworkObjIsDuplicate(&driver->networks, def, 1) < 0) + goto cleanup; + if (virNetworkSetBridgeName(&driver->networks, def, 1)) goto cleanup;
@@ -1295,12 +1298,16 @@ static virNetworkPtr networkDefine(virConnectPtr conn, const char *xml) { virNetworkDefPtr def; virNetworkObjPtr network = NULL; virNetworkPtr ret = NULL; + int dupNet;
Why not just check the error code directly like in the other use?
Aside from that, ACK
I was just copying the code from the QEMU driver, which uses this variable to emit events a short while later. We need to make the network driver emit events too..... 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 :|
participants (3)
-
Cole Robinson -
Daniel P. Berrange -
Dave Allan