Save and restore node names if we know them in the status XML so that we
don't need to recalculate them or don't lose them in some cases.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/qemu/qemu_domain.c                    | 12 ++++++++++++
 tests/qemustatusxml2xmldata/modern-in.xml |  4 ++++
 2 files changed, 16 insertions(+)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 326c939c85..2c5de9508b 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1937,6 +1937,9 @@ static int
 qemuStorageSourcePrivateDataParse(xmlXPathContextPtr ctxt,
                                   virStorageSourcePtr src)
 {
+    src->nodestorage = virXPathString("string(./nodenames/nodename[@type='storage']/@name)", ctxt);
+    src->nodeformat = virXPathString("string(./nodenames/nodename[@type='format']/@name)", ctxt);
+
     if (virStorageSourcePrivateDataParseRelPath(ctxt, src) < 0)
         return -1;

@@ -1948,6 +1951,15 @@ static int
 qemuStorageSourcePrivateDataFormat(virStorageSourcePtr src,
                                    virBufferPtr buf)
 {
+    if (src->nodestorage || src->nodeformat) {
+        virBufferAddLit(buf, "<nodenames>\n");
+        virBufferAdjustIndent(buf, 2);
+        virBufferEscapeString(buf, "<nodename type='storage' name='%s'/>\n", src->nodestorage);
+        virBufferEscapeString(buf, "<nodename type='format' name='%s'/>\n", src->nodeformat);
+        virBufferAdjustIndent(buf, -2);
+        virBufferAddLit(buf, "</nodenames>\n");
+    }
+
     if (virStorageSourcePrivateDataFormatRelPath(src, buf) < 0)
         return -1;

diff --git a/tests/qemustatusxml2xmldata/modern-in.xml b/tests/qemustatusxml2xmldata/modern-in.xml
index c1e57618b6..d57e1f605f 100644
--- a/tests/qemustatusxml2xmldata/modern-in.xml
+++ b/tests/qemustatusxml2xmldata/modern-in.xml
@@ -309,6 +309,10 @@
           <format type='qcow2'/>
           <source file='/var/lib/libvirt/images/base.qcow2'>
             <privateData>
+              <nodenames>
+                <nodename type='storage' name='test-storage'/>
+                <nodename type='format' name='test-format'/>
+              </nodenames>
               <relPath>base.qcow2</relPath>
             </privateData>
           </source>
-- 
2.16.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 04/25/2018 11:15 AM, Peter Krempa wrote:
> Save and restore node names if we know them in the status XML so that we
> don't need to recalculate them or don't lose them in some cases.
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  src/qemu/qemu_domain.c                    | 12 ++++++++++++
>  tests/qemustatusxml2xmldata/modern-in.xml |  4 ++++
>  2 files changed, 16 insertions(+)

So, what would cause a need for recalculation?  Or losing them?

I assume it's "expensive" to query, but then I wonder what trap we could
fall into by saving them. If we've needed to recalculate or lost them
before, what's to say that same issue doesn't happen by saving them?

I also assume they don't change after we've saved them, right? I'm not
opposed to this, it's the OK so how are they to be used and then why if
they could be lost or needed recalculation would we want to save them?
Also, what assurances do we have then when they are to be saved that
they aren't in a condition that caused this patch to be generated.

If the only way to get the 'real' value is ask QEMU, then why not ask?
It's not like an alias which we generated it's IIUC something QEMU
generates and wishes to be referenced in that manner.

Maybe it's just the wording of the commit that raised the questions ;-)

John

> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 326c939c85..2c5de9508b 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1937,6 +1937,9 @@ static int
>  qemuStorageSourcePrivateDataParse(xmlXPathContextPtr ctxt,
>                                    virStorageSourcePtr src)
>  {
> +    src->nodestorage = virXPathString("string(./nodenames/nodename[@type='storage']/@name)", ctxt);
> +    src->nodeformat = virXPathString("string(./nodenames/nodename[@type='format']/@name)", ctxt);
> +
>      if (virStorageSourcePrivateDataParseRelPath(ctxt, src) < 0)
>          return -1;
> 
> @@ -1948,6 +1951,15 @@ static int
>  qemuStorageSourcePrivateDataFormat(virStorageSourcePtr src,
>                                     virBufferPtr buf)
>  {
> +    if (src->nodestorage || src->nodeformat) {
> +        virBufferAddLit(buf, "<nodenames>\n");
> +        virBufferAdjustIndent(buf, 2);
> +        virBufferEscapeString(buf, "<nodename type='storage' name='%s'/>\n", src->nodestorage);
> +        virBufferEscapeString(buf, "<nodename type='format' name='%s'/>\n", src->nodeformat);
> +        virBufferAdjustIndent(buf, -2);
> +        virBufferAddLit(buf, "</nodenames>\n");
> +    }
> +
>      if (virStorageSourcePrivateDataFormatRelPath(src, buf) < 0)
>          return -1;
> 
> diff --git a/tests/qemustatusxml2xmldata/modern-in.xml b/tests/qemustatusxml2xmldata/modern-in.xml
> index c1e57618b6..d57e1f605f 100644
> --- a/tests/qemustatusxml2xmldata/modern-in.xml
> +++ b/tests/qemustatusxml2xmldata/modern-in.xml
> @@ -309,6 +309,10 @@
>            <format type='qcow2'/>
>            <source file='/var/lib/libvirt/images/base.qcow2'>
>              <privateData>
> +              <nodenames>
> +                <nodename type='storage' name='test-storage'/>
> +                <nodename type='format' name='test-format'/>
> +              </nodenames>
>                <relPath>base.qcow2</relPath>
>              </privateData>
>            </source>
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, May 01, 2018 at 20:06:43 -0400, John Ferlan wrote:
> 
> 
> On 04/25/2018 11:15 AM, Peter Krempa wrote:
> > Save and restore node names if we know them in the status XML so that we
> > don't need to recalculate them or don't lose them in some cases.
> > 
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > ---
> >  src/qemu/qemu_domain.c                    | 12 ++++++++++++
> >  tests/qemustatusxml2xmldata/modern-in.xml |  4 ++++
> >  2 files changed, 16 insertions(+)
> 
> So, what would cause a need for recalculation?  Or losing them?
> 
> I assume it's "expensive" to query, but then I wonder what trap we could
> fall into by saving them. If we've needed to recalculate or lost them
> before, what's to say that same issue doesn't happen by saving them?

Well. Currently we use node-names only for the 'block-threshold' event
and we get them by querying them from qemu. Since they are not stored in
the XML a libvirtd restart requires us to re-query them.

In the future we will start generating and providing our own node names
to the block layer, so it will be required that we remember them.

Both re-generating and detecting is way more complicated than just
storing them.

> I also assume they don't change after we've saved them, right? I'm not
> opposed to this, it's the OK so how are they to be used and then why if
> they could be lost or needed recalculation would we want to save them?
> Also, what assurances do we have then when they are to be saved that
> they aren't in a condition that caused this patch to be generated.

No, they just are lost on restart.

> If the only way to get the 'real' value is ask QEMU, then why not ask?
> It's not like an alias which we generated it's IIUC something QEMU
> generates and wishes to be referenced in that manner.
> 
> Maybe it's just the wording of the commit that raised the questions ;-)

Umm probably yes. I've had this commit around for a long time :)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 05/02/2018 05:51 AM, Peter Krempa wrote:
> On Tue, May 01, 2018 at 20:06:43 -0400, John Ferlan wrote:
>>
>>
>> On 04/25/2018 11:15 AM, Peter Krempa wrote:
>>> Save and restore node names if we know them in the status XML so that we
>>> don't need to recalculate them or don't lose them in some cases.
>>>
>>> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>>> ---
>>>  src/qemu/qemu_domain.c                    | 12 ++++++++++++
>>>  tests/qemustatusxml2xmldata/modern-in.xml |  4 ++++
>>>  2 files changed, 16 insertions(+)
>>
>> So, what would cause a need for recalculation?  Or losing them?
>>
>> I assume it's "expensive" to query, but then I wonder what trap we could
>> fall into by saving them. If we've needed to recalculate or lost them
>> before, what's to say that same issue doesn't happen by saving them?
> 
> Well. Currently we use node-names only for the 'block-threshold' event
> and we get them by querying them from qemu. Since they are not stored in
> the XML a libvirtd restart requires us to re-query them.
> 
> In the future we will start generating and providing our own node names
> to the block layer, so it will be required that we remember them.

Since we can re-query, then the real need to save them is to avoid the
[time consuming, complicated] re-query - which is fine as long as they
stay in sync!

> 
> Both re-generating and detecting is way more complicated than just
> storing them.
> 
>> I also assume they don't change after we've saved them, right? I'm not
>> opposed to this, it's the OK so how are they to be used and then why if
>> they could be lost or needed recalculation would we want to save them?
>> Also, what assurances do we have then when they are to be saved that
>> they aren't in a condition that caused this patch to be generated.
> 
> No, they just are lost on restart.
> 

I was too lazy to look last night 0-). I see qemuBlockNodeNamesDetect is
called during Reconnect which seems to fill in those values.

>> If the only way to get the 'real' value is ask QEMU, then why not ask?
>> It's not like an alias which we generated it's IIUC something QEMU
>> generates and wishes to be referenced in that manner.
>>
>> Maybe it's just the wording of the commit that raised the questions ;-)
> 
> Umm probably yes. I've had this commit around for a long time :)
> 

In any case, thanks for the explanation. Then it's just some commit
message wordsmithing to indicate we'll save to avoid the re-query. The
above function already avoids re-query if nodeformat or nodestorage is
already defined.

Thus,

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list