On Fri, Oct 31, 2008 at 12:58:17PM +0100, Chris Lalancette wrote:
> Daniel P. Berrange wrote:
> >> Personally, I think those are bad semantics for virStorageBackendStablePath;
> >> assuming it succeeds, you should always be able to know that you have a copy,
> >> regardless of whether the copy is the same as the original. Should I change
> >> virStorageBackendStablePath to those semantics, in which case your below code
> >> would then be correct?
> >
> > Yes, I think that's worth doing - will also avoid the cast in the input
> > arg there
>
> OK, updated patch attached; virStorageBackendStablePath now always returns a
> copy of the given string, so it's always safe to unconditionally VIR_FREE it. I
> fixed up storage_backend_iscsi and storage_backend_disk to reflect this change.
> I also re-worked the code as you suggested, and added a bit more error checking.
>
> Index: src/storage_backend.c
> ===================================================================
> RCS file: /data/cvs/libvirt/src/storage_backend.c,v
> retrieving revision 1.24
> diff -u -r1.24 storage_backend.c
> --- src/storage_backend.c 28 Oct 2008 17:48:06 -0000 1.24
> +++ src/storage_backend.c 31 Oct 2008 11:56:33 -0000
> @@ -357,7 +357,7 @@
> char *
> virStorageBackendStablePath(virConnectPtr conn,
> virStoragePoolObjPtr pool,
> - char *devpath)
> + const char *devpath)
> {
> DIR *dh;
> struct dirent *dent;
> @@ -366,7 +366,7 @@
> if (pool->def->target.path == NULL ||
> STREQ(pool->def->target.path, "/dev") ||
> STREQ(pool->def->target.path, "/dev/"))
> - return devpath;
> + return strdup(devpath);
Need to call virStorageReportError here on OOM.
>
> /* The pool is pointing somewhere like /dev/disk/by-path
> * or /dev/disk/by-id, so we need to check all symlinks in
> @@ -410,7 +410,7 @@
> /* Couldn't find any matching stable link so give back
> * the original non-stable dev path
> */
> - return devpath;
> + return strdup(devpath);
And here.
Since virStorageBackendStablePath() api contract says that it is responsible
for setting the errors upon failure.
> Index: src/storage_driver.c
> ===================================================================
> RCS file: /data/cvs/libvirt/src/storage_driver.c,v
> retrieving revision 1.13
> diff -u -r1.13 storage_driver.c
> --- src/storage_driver.c 21 Oct 2008 17:15:53 -0000 1.13
> +++ src/storage_driver.c 31 Oct 2008 11:56:34 -0000
> @@ -966,8 +966,34 @@
>
> for (i = 0 ; i < driver->pools.count ; i++) {
> if (virStoragePoolObjIsActive(driver->pools.objs[i])) {
> - virStorageVolDefPtr vol =
> - virStorageVolDefFindByPath(driver->pools.objs[i], path);
> + virStorageVolDefPtr vol;
> + virStorageBackendPoolOptionsPtr options;
> +
> + options = virStorageBackendPoolOptionsForType(driver->pools.objs[i]->def->type);
> + if (options == NULL)
> + continue;
> +
> + if (options->flags & VIR_STORAGE_BACKEND_POOL_STABLE_PATH) {
> + const char *stable_path;
> +
> + stable_path = virStorageBackendStablePath(conn,
> + driver->pools.objs[i],
> + path);
> + /*
> + * virStorageBackendStablePath already does
> + * virStorageReportError if it fails; we just need to keep
> + * propagating the return code
> + */
> + if (stable_path == NULL)
> + return NULL;
> +
> + vol = virStorageVolDefFindByPath(driver->pools.objs[i],
> + stable_path);
> + VIR_FREE(stable_path);
> + }
> + else
> + vol = virStorageVolDefFindByPath(driver->pools.objs[i], path);
> +
>
> if (vol)
> return virGetStorageVol(conn,
This looks good now.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.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