Re: [libvirt] [PATCH] Make logical pools independent on target path
On 06/03/2013 06:22 PM, Osier Yang wrote:
On 03/06/13 23:05, Martin Kletzander wrote:
> When using logical pools, we had to trust the target->pth provided.
s/pth/path/,
> This parameter, however, can be completely ommited and we can get the
> correct path using 'lvdisplay' command.
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=952973
>
> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
> ---
>
> Notes:
> I'm not sure we can drop the target.path from the XML (see
> tests), so
> another variant is to keep it there the same way it was defined by
> user/mgmt app.
How about one file a bug "The created volume is not under specified pool's
target path"? Since with this patch, the specified pool's target path is
silently
ignored.
If you have a good answer/reason/fix for above question, I see no
problem of
ignore the pool->target.path, but the path you got from lvdisplay (with
vol name
is truncated) should be reflected to the pool's XML.
Sorry for such late reply, but I didn't get to this issue until now.
The problem about updating the target.path is that the function that deals with the pool
creation gets different copy of data that the one that builds the pool. I already forgot
why I haven't rewritten the whole mess in a way that allows the changing of parameters
even in the building function, so I'll dig into that and maybe seeing the problem
after a while will help and I'll get this fixed.
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index cc3d3d9..948e190 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -1,7 +1,7 @@
> /*
> * storage_conf.c: config handling for storage driver
> *
> - * Copyright (C) 2006-2012 Red Hat, Inc.
> + * Copyright (C) 2006-2013 Red Hat, Inc.
> * Copyright (C) 2006-2008 Daniel P. Berrange
> *
> * This library is free software; you can redistribute it and/or
> @@ -947,15 +947,16 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
> /* When we are working with a virtual disk we can skip the target
> * path and permissions */
> if (!(options->flags & VIR_STORAGE_POOL_SOURCE_NETWORK)) {
> - if (!(target_path = virXPathString("string(./target/path)",
> ctxt))) {
> - virReportError(VIR_ERR_XML_ERROR, "%s",
> - _("missing storage pool target path"));
> - goto error;
> + if (ret->type != VIR_STORAGE_POOL_LOGICAL) {
> + if (!(target_path =
> virXPathString("string(./target/path)", ctxt))) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("missing storage pool target path"));
> + goto error;
> + }
> + ret->target.path = virFileSanitizePath(target_path);
> + if (!ret->target.path)
> + goto error;
RNG schema needs to be changed too..
You mean making the target.path optional? I think it is already. Or did you mean
something else?
[...]
> @@ -314,7 +340,7 @@
> virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool,
> "--unbuffered",
> "--nosuffix",
> "--options",
> -
>
"lv_name,origin,uuid,devices,segtype,stripes,seg_size,vg_extent_size,size,lv_attr",
>
I won't want to see what I added (lv_attr) is removed by mistake, :-)
I have no idea how that happened. I probably missed that it is used to skip inactive
volumes. Will fix that
[...]
> diff --git a/tests/storagepoolxml2xmlin/pool-logical-create.xml
> b/tests/storagepoolxml2xmlin/pool-logical-create.xml
> index 4c67089..e1bb4db 100644
> --- a/tests/storagepoolxml2xmlin/pool-logical-create.xml
> +++ b/tests/storagepoolxml2xmlin/pool-logical-create.xml
> @@ -10,7 +10,6 @@
> <device path="/dev/sdb3"/>
> </source>
> <target>
> - <path>/dev/HostVG</path>
At least one test should have the specified target path, to make sure
the old XML still work..
Good point, but I'll modify the tests accordingly to the change anyway ;-)
Martin