On Thu, Sep 01, 2011 at 02:49:04PM +0800, Lei Li wrote:
> Fix bug #611823 storage driver should prohibit pools with duplicate underlying
> storage.
>
> Add API virStoragePoolSourceFindDuplicate() to do uniqueness check based on
> source location infomation for pool type.
>
>
> Signed-off-by: Lei Li <lilei@xxxxxxxxxxxxxxxxxx>
> ---
> src/conf/storage_conf.c | 73 ++++++++++++++++++++++++++++++++++++++++++
> src/conf/storage_conf.h | 3 ++
> src/libvirt_private.syms | 1 +
> src/storage/storage_driver.c | 6 +++
> 4 files changed, 83 insertions(+), 0 deletions(-)
>
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index 8d14e87..698334e 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -1701,6 +1701,79 @@ cleanup:
> return ret;
> }
>
> +int virStoragePoolSourceFindDuplicate(virStoragePoolObjListPtr pools,
> + virStoragePoolDefPtr def)
> +{
> + int i, j, k;
> + int ret = 1;
> + virStoragePoolObjPtr pool = NULL;
> + virStoragePoolObjPtr matchpool = NULL;
> +
> + /* Check the pool list for duplicate underlying storage */
> + for (i = 0; i < pools->count; i++) {
> + pool = pools->objs[i];
> + if (def->type != pool->def->type)
> + continue;
> +
> + virStoragePoolObjLock(pool);
> + if (pool->def->type == VIR_STORAGE_POOL_DIR) {
> + if (STREQ(pool->def->target.path, def->target.path)) {
> + matchpool = pool;
> + goto cleanup;
> + }
> + } else if (pool->def->type == VIR_STORAGE_POOL_NETFS) {
> + if ((STREQ(pool->def->source.dir, def->source.dir)) \
> + && (STREQ(pool->def->source.host.name, def->source.host.name))) {
> + matchpool = pool;
> + goto cleanup;
> + }
> + } else if (pool->def->type == VIR_STORAGE_POOL_SCSI) {
> + if (STREQ(pool->def->source.adapter, def->source.adapter)) {
> + matchpool = pool;
> + goto cleanup;
> + }
> + } else if (pool->def->type == VIR_STORAGE_POOL_ISCSI) {
> + for (j = 0; j < pool->def->source.ndevice; j++) {
> + for (k = 0; k < def->source.ndevice; k++) {
> + if (pool->def->source.initiator.iqn) {
> + if ((STREQ(pool->def->source.devices[j].path, def->source.devices[k].path)) \
> + && (STREQ(pool->def->source.initiator.iqn, def->source.initiator.iqn))) {
> + matchpool = pool;
> + goto cleanup;
> + }
> + } else if (pool->def->source.host.name) {
> + if ((STREQ(pool->def->source.devices[j].path, def->source.devices[k].path)) \
> + && (STREQ(pool->def->source.host.name, def->source.host.name))) {
> + matchpool = pool;
> + goto cleanup;
> + }
> + }
Slight change needed here. The 'source.host.name' is compulsory and always
present for iSCSI. So you always need to comper path + host.name. The IQN
is optional, so should onlybe compared if it is present in *both* pools.
Your current code can SEGV if def->source.initiator.iqn is NULL.
> + } else {
> + /* For the remain three pool type 'fs''logical''disk' that all use device path */
We might add further pool types later, and we don't want this branch accidentally
executed for them. I think it would thus be preferrable to turn this long if/else
into a switch() block. Then explicitly list FS, LOGICAL & DISK here, and have a
separate 'default:' block which is a no-op.
> + if (pool->def->source.ndevice) {
> + for (j = 0; j < pool->def->source.ndevice; j++) {
> + for (k = 0; k < def->source.ndevice; k++) {
> + if (STREQ(pool->def->source.devices[j].path, def->source.devices[k].path)) {
> + matchpool = pool;
> + goto cleanup;
> + }
> + }
> + }
> + }
> + }
> + virStoragePoolObjUnlock(pool);
> + }
> +cleanup:
> + if (matchpool) {
> + virStoragePoolObjUnlock(matchpool);
> + virStorageReportError(VIR_ERR_OPERATION_FAILED,
> + _("pool source location info is duplicate"));
> + ret = -1;
> + }
> + return ret;
> +}
Aside from those comments, this looks like the right approach to the
problem
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list