Re: [libvirt] [PATCH v3 04/18] conf: Rework parsing in virStoragePoolDefParseSourceAdapter
On 03/11/2017 08:16 PM, Laine Stump wrote:
On 03/10/2017 04:10 PM, John Ferlan wrote:
> Rather than use virXPathString, pass along an virXPathNode and alter
> the parsing to use virXMLPropString.
Just so I understand the reasoning correctly - you're not doing this so you can use
virXMLPropString() instead of virXPathString(), but just so you can remove the
"adapter" from the path to each attribute (and in the cases where that turns a
"path" into simply the attribute name, you're switching to
virXMLPropString() because it's presumably slightly more efficient. Right? Or is there
some other reason you prefer virXMLPropString()?
Missed this on the first pass through your review... Probably because
your email client has stopped believing in line wrapping or my client
isn't seeing some setting properly <sigh>
I had originally wanted the ability to just parse an <adapter...> since
that was how this would be represented in the domain; however, even
though that got mothballed - I still felt what I'd done so far was at
least a step up in readability, flow, etc. over what was there before.
Secondary to that I find it easier to "read" the code when using
virXMLPropString instead of virXPathString
John
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/conf/storage_conf.c | 51 ++++++++++++++++++++++++++-----------------------
> 1 file changed, 27 insertions(+), 24 deletions(-)
>
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index 1993d3a..4fa7c12 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -464,13 +464,17 @@ virStoragePoolObjRemove(virStoragePoolObjListPtr pools,
>
> static int
> virStoragePoolDefParseSourceAdapter(virStoragePoolSourcePtr source,
> + xmlNodePtr node,
> xmlXPathContextPtr ctxt)
> {
> int ret = -1;
> + xmlNodePtr relnode = ctxt->node;
(This is the first time I've seen "relnode" used to save a ctxt->node.
When I look through the source, the one other place I see it is in virxml.c (other places
use "node", "save_node", "oldnode", "save_ctxt",
"tmpnode", and on an on). Now I'm trying to figure out what "rel"
stands for but nothing comes to mind. How stupid am I?)
> [...]
ACK.