Changeset
docs/formatdomain.html.in                      |  36 ++++-
docs/schemas/domaincommon.rng                  | 108 ++++++++++---
src/bhyve/bhyve_command.c                      |   6 +-
src/conf/domain_conf.c                         | 215 +++++++++++++++++++++++--
src/conf/domain_conf.h                         |  11 +-
src/qemu/qemu_cgroup.c                         |  13 +-
src/qemu/qemu_command.c                        |  21 ++-
src/qemu/qemu_domain.c                         |  31 ++--
src/qemu/qemu_driver.c                         |   7 +-
src/qemu/qemu_parse_command.c                  |  30 +++-
src/qemu/qemu_process.c                        |  42 +++--
src/security/security_dac.c                    |   6 +-
src/security/security_selinux.c                |   6 +-
src/security/virt-aa-helper.c                  |  14 +-
src/vbox/vbox_common.c                         |  11 +-
src/xenapi/xenapi_driver.c                     |   4 +-
src/xenconfig/xen_sxpr.c                       |  19 ++-
src/xenconfig/xen_xm.c                         |   9 +-
tests/qemuxml2argvdata/bios-nvram-network.args |  31 ++++
tests/qemuxml2argvdata/bios-nvram-network.xml  |  42 +++++
tests/qemuxml2argvtest.c                       |   1 +
21 files changed, 562 insertions(+), 101 deletions(-)
create mode 100644 tests/qemuxml2argvdata/bios-nvram-network.args
create mode 100644 tests/qemuxml2argvdata/bios-nvram-network.xml
Git apply log
Switched to a new branch '20180514111522.4363-1-saxenap.ltc@gmail.com'
Applying: Schema: Introduce XML schema for network-backed loader and nvram elements.
Applying: Loader: Add a more elaborate definition.
Applying: Parse domain XML to generate virDomainLoaderDef & virDomainNvramDef.
Applying: Format the loader source appropriately.
Applying: Plumb the loader source into generation of QEMU command line.
Applying: Fix the domain def inference logic to correctly account for network-backed pflash devices.
Applying: Bhyve: Fix command line generation to correctly pick up local loader path.
Applying: virt-aa-helper: Adjust references to loader & nvram elements to correctly parse the virStorageSource types.
Applying: Vbox: Adjust references to 'loader' and 'nvram' elements given that these are now represented by virStorageSourcePtr.
Applying: Xen: Adjust all references to loader & nvram elements given that they are now backed by virStorageSourcePtr
Applying: Test: Add a test snippet to evaluate command line generation for loader/nvram specified via virStorageSource
Applying: Documentation: Add a blurb for the newly added XML snippets for loader and nvram.
To https://github.com/patchew-project/libvirt
 * [new tag]         patchew/20180514111522.4363-1-saxenap.ltc@gmail.com -> patchew/20180514111522.4363-1-saxenap.ltc@gmail.com
Test failed: syntax-check

loading

[libvirt] [PATCH 00/12][v2] Introduce network-backed loader & nvram.
Posted by Prerna Saxena, 1 week ago
Libvirt domain XML today only allows local filepaths that can be
used to specify a loader element or its matching NVRAM disk.
Given that Vms may themselves move across hypervisor hosts, it should be
possible to allocate loaders/NVRAM disks on network storage for
uninterrupted access.

This series extends the firmware loader & NVRAM disks to be hosted over
network-backed disks, for high availability.
It achieves this by embedding virStorageSource elements for loader &
nvram into _virDomainLoaderDef, as discussed in
https://www.redhat.com/archives/libvir-list/2018-March/msg01721.html.

Currently, the source type is annotated by introducing a new attribute "backing" for both
'loader' and 'nvram' elements. Hence, a sample XML with new annotation looks like this:

<loader readonly='yes' type='pflash' backing='file'>
  <source file='/usr/share/OVMF/OVMF_CODE.fd'/>
</loader>
<nvram backing='network'>  <source protocol='iscsi' name='iqn.2013-07.com.example:iscsi-nopool/0'>
  <host name='example.com' port='6000'/>
</source>

References:
----------
v0 / Proposal: https://www.redhat.com/archives/libvir-list/2018-March/msg01721.html.
v1 : https://www.redhat.com/archives/libvir-list/2018-April/msg02024.html 

Changelog:
---------
Changes since v1:
1. Split up the patch into smaller units.
2. Added doc snippet & tests.
3. Changed the formatting code in patch 4 to format a domain's XML in
the style that was read.

I found that encryption seems to be a property of the storage volume.
I didnt  include that in this series since it does not use backing type
as volume. Will include that later once the basic network support
patches get done.

Looking forward to feedback,
Prerna

Prerna Saxena (12):
  Schema: Introduce XML schema for network-backed loader and nvram
    elements.
  Loader: Add a more elaborate definition.
  Parse domain XML to generate virDomainLoaderDef & virDomainNvramDef.
  Format the loader source appropriately.
  Plumb the loader source into generation of QEMU command line.
  Fix the domain def inference logic to correctly account for
    network-backed     pflash devices.
  Bhyve: Fix command line generation to correctly pick up local loader
    path.
  virt-aa-helper: Adjust references to loader & nvram elements to
    correctly     parse the virStorageSource types.
  Vbox: Adjust references to 'loader' and 'nvram' elements given that   
     these are now represented by virStorageSourcePtr.
  Xen: Adjust all references to loader & nvram elements given that they
    are now backed by virStorageSourcePtr
  Test: Add a test snippet to evaluate command line generation for    
    loader/nvram specified via virStorageSource
  Documentation: Add a blurb for the newly added XML snippets for loader
    and nvram.

 docs/formatdomain.html.in                      |  36 ++++-
 docs/schemas/domaincommon.rng                  | 108 ++++++++++---
 src/bhyve/bhyve_command.c                      |   6 +-
 src/conf/domain_conf.c                         | 215 +++++++++++++++++++++++--
 src/conf/domain_conf.h                         |  11 +-
 src/qemu/qemu_cgroup.c                         |  13 +-
 src/qemu/qemu_command.c                        |  21 ++-
 src/qemu/qemu_domain.c                         |  31 ++--
 src/qemu/qemu_driver.c                         |   7 +-
 src/qemu/qemu_parse_command.c                  |  30 +++-
 src/qemu/qemu_process.c                        |  42 +++--
 src/security/security_dac.c                    |   6 +-
 src/security/security_selinux.c                |   6 +-
 src/security/virt-aa-helper.c                  |  14 +-
 src/vbox/vbox_common.c                         |  11 +-
 src/xenapi/xenapi_driver.c                     |   4 +-
 src/xenconfig/xen_sxpr.c                       |  19 ++-
 src/xenconfig/xen_xm.c                         |   9 +-
 tests/qemuxml2argvdata/bios-nvram-network.args |  31 ++++
 tests/qemuxml2argvdata/bios-nvram-network.xml  |  42 +++++
 tests/qemuxml2argvtest.c                       |   1 +
 21 files changed, 562 insertions(+), 101 deletions(-)
 create mode 100644 tests/qemuxml2argvdata/bios-nvram-network.args
 create mode 100644 tests/qemuxml2argvdata/bios-nvram-network.xml

-- 
1.8.1.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 01/12] Schema: Introduce XML schema for network-backed loader and nvram elements.
Posted by Prerna Saxena, 1 week ago
Today, 'loader' and 'nvram' elements are supposed to be backed by a local disk.
Given that NVRAM data could be network-backed for high availability, this patch
defines XML spec for serving loader & nvram disks via the network.

Signed-off-by: Prerna Saxena <saxenap.ltc@gmail.com>
---
 docs/schemas/domaincommon.rng | 108 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 90 insertions(+), 18 deletions(-)

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 0a6b29b..a6207db 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -276,7 +276,42 @@
                 </choice>
               </attribute>
             </optional>
-            <ref name="absFilePath"/>
+            <optional>
+              <attribute name="backing">
+                <choice>
+                  <value>file</value>
+                  <value>network</value>
+                </choice>
+              </attribute>
+            </optional>
+            <optional>
+              <choice>
+                 <group>
+                  <ref name="absFilePath"/>
+                 </group>
+                 <group>
+                  <ref name="diskSourceFileElement"/>
+                 </group>
+                 <group>
+                  <ref name="diskSourceNetworkProtocolNBD"/>
+                 </group>
+                 <group>
+                  <ref name="diskSourceNetworkProtocolGluster"/>
+                 </group>
+                 <group>
+                  <ref name="diskSourceNetworkProtocolRBD"/>
+                 </group>
+                 <group>
+                  <ref name="diskSourceNetworkProtocolISCSI"/>
+                 </group>
+                 <group>
+                  <ref name="diskSourceNetworkProtocolHTTP"/>
+                 </group>
+                 <group>
+                  <ref name="diskSourceNetworkProtocolSimple"/>
+                 </group>
+              </choice>
+            </optional>
           </element>
         </optional>
         <optional>
@@ -287,7 +322,40 @@
               </attribute>
             </optional>
             <optional>
-              <ref name="absFilePath"/>
+              <attribute name="backing">
+                <choice>
+                  <value>file</value>
+                  <value>network</value>
+                </choice>
+              </attribute>
+            </optional>
+            <optional>
+              <choice>
+                 <group>
+                  <ref name="absFilePath"/>
+                 </group>
+                 <group>
+                  <ref name="diskSourceFileElement"/>
+                 </group>
+                 <group>
+                  <ref name="diskSourceNetworkProtocolNBD"/>
+                 </group>
+                 <group>
+                  <ref name="diskSourceNetworkProtocolGluster"/>
+                 </group>
+                 <group>
+                  <ref name="diskSourceNetworkProtocolRBD"/>
+                 </group>
+                 <group>
+                  <ref name="diskSourceNetworkProtocolISCSI"/>
+                 </group>
+                 <group>
+                  <ref name="diskSourceNetworkProtocolHTTP"/>
+                 </group>
+                 <group>
+                  <ref name="diskSourceNetworkProtocolSimple"/>
+                 </group>
+              </choice>
             </optional>
           </element>
         </optional>
@@ -1494,25 +1562,29 @@
       </attribute>
     </optional>
     <optional>
-      <element name="source">
-        <optional>
-          <attribute name="file">
-            <ref name="absFilePath"/>
-          </attribute>
-        </optional>
-        <optional>
-          <ref name="storageStartupPolicy"/>
-        </optional>
-        <optional>
-          <ref name="encryption"/>
-        </optional>
-        <zeroOrMore>
-          <ref name='devSeclabel'/>
-        </zeroOrMore>
-      </element>
+      <ref name='diskSourceFileElement'/>
     </optional>
   </define>
 
+  <define name="diskSourceFileElement">
+    <element name="source">
+      <optional>
+        <attribute name="file">
+          <ref name="absFilePath"/>
+        </attribute>
+      </optional>
+      <optional>
+        <ref name="storageStartupPolicy"/>
+      </optional>
+      <optional>
+        <ref name="encryption"/>
+      </optional>
+      <zeroOrMore>
+        <ref name='devSeclabel'/>
+      </zeroOrMore>
+    </element>
+  </define>
+
   <define name="diskSourceBlock">
     <attribute name="type">
       <value>block</value>
-- 
1.8.1.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 01/12] Schema: Introduce XML schema for network-backed loader and nvram elements.
Posted by John Ferlan, 6 days ago
$SUBJ:

Is a bit long - goal is <= 70-ish characters

On 05/14/2018 07:15 AM, Prerna Saxena wrote:
> Today, 'loader' and 'nvram' elements are supposed to be backed by a local disk.
> Given that NVRAM data could be network-backed for high availability, this patch
> defines XML spec for serving loader & nvram disks via the network.
> 
> Signed-off-by: Prerna Saxena <saxenap.ltc@gmail.com>
> ---
>  docs/schemas/domaincommon.rng | 108 +++++++++++++++++++++++++++++++++++-------
>  1 file changed, 90 insertions(+), 18 deletions(-)
> 

First off: applying all the patches and running make w/ "check" and
"syntax-check" fails during "check" w/ qemuxml2argvtest and
qemuxml2xmltest failing.

Before posting patches those must pass for each patch. When I get to
patch 2, the build breaks. While one can appreciate having less to
review in each patch, it's not possible to accept or review patches
which cause a build break. Shouldn't be up to the reviewer to figure out
how to make the series work. The rule is - each patch must be separately
compileable and capable of passing check and syntax-check. If one ever
needs to git bisect to determine where a problem lies, it'd be very
awful if the build broke.

As for this patch in particular, there are two things going on in this
patch - #1 altering the <loader> and <nvram> schema and #2 extracting
out part of the diskSourceFile to be used in #1.  Ironically, Thing 2 is
creating a referenced name to be included in part of Thing 1; however,
Thing 1 is essentially a cut-n-paste of the same thing. All those
elements that are cut-n-pasted are part of diskSourceNetwork, except you
didn't include VxHS in your list.

Still, a question in my mind is whether you really need to format the
file using source. If the goal is to provide the ability to access
networked storage, why provide the option to allow someone to change
their XML from:

<loader type='rom'>/usr/lib/xen/boot/hvmloader</loader>

to

<loader type='rom' backing='file'>
  <source file='/usr/lib/xen/boot/hvmloader'/>
</loader>

Yes, they are equivalent, but it seems the reason for it was because you
wanted to format the former into the latter at one point in time.

If you limit to network only, then perhaps your optional attribute
changes to be network='yes' which means to parse a <source> which may
clear up some of the "odd" pieces of the grammar below.

> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 0a6b29b..a6207db 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -276,7 +276,42 @@
>                  </choice>
>                </attribute>
>              </optional>
> -            <ref name="absFilePath"/>
> +            <optional>
> +              <attribute name="backing">
> +                <choice>
> +                  <value>file</value>
> +                  <value>network</value>
> +                </choice>
> +              </attribute>
> +            </optional>
> +            <optional>
> +              <choice>
> +                 <group>
> +                  <ref name="absFilePath"/>
> +                 </group>

This won't be equivalent to what you started with. Prior to this change
absFilePath was required, now it would be optional.

I would assume that if it's not optional here for absFilePath, then the
<source> cannot be optional as well.

> +                 <group>
> +                  <ref name="diskSourceFileElement"/>
> +                 </group>
> +                 <group>
> +                  <ref name="diskSourceNetworkProtocolNBD"/>
> +                 </group>
> +                 <group>
> +                  <ref name="diskSourceNetworkProtocolGluster"/>
> +                 </group>
> +                 <group>
> +                  <ref name="diskSourceNetworkProtocolRBD"/>
> +                 </group>
> +                 <group>
> +                  <ref name="diskSourceNetworkProtocolISCSI"/>
> +                 </group>
> +                 <group>
> +                  <ref name="diskSourceNetworkProtocolHTTP"/>
> +                 </group>
> +                 <group>
> +                  <ref name="diskSourceNetworkProtocolSimple"/>
> +                 </group>
> +              </choice>
> +            </optional>
>            </element>
>          </optional>
>          <optional>
> @@ -287,7 +322,40 @@
>                </attribute>
>              </optional>
>              <optional>
> -              <ref name="absFilePath"/>
> +              <attribute name="backing">
> +                <choice>
> +                  <value>file</value>
> +                  <value>network</value>
> +                </choice>
> +              </attribute>
> +            </optional>
> +            <optional>
> +              <choice>
> +                 <group>
> +                  <ref name="absFilePath"/>
> +                 </group>

This is slightly different as absFilePath is optional...

> +                 <group>
> +                  <ref name="diskSourceFileElement"/>
> +                 </group>
> +                 <group>
> +                  <ref name="diskSourceNetworkProtocolNBD"/>
> +                 </group>
> +                 <group>
> +                  <ref name="diskSourceNetworkProtocolGluster"/>
> +                 </group>
> +                 <group>
> +                  <ref name="diskSourceNetworkProtocolRBD"/>
> +                 </group>
> +                 <group>
> +                  <ref name="diskSourceNetworkProtocolISCSI"/>
> +                 </group>
> +                 <group>
> +                  <ref name="diskSourceNetworkProtocolHTTP"/>
> +                 </group>
> +                 <group>
> +                  <ref name="diskSourceNetworkProtocolSimple"/>
> +                 </group>
> +              </choice>



RNG format not my area of expertise - I usually copy, paste, and pray it
works. Still the above looks really strange with all those <group> ..
</group>... After a bit of playing I came up with the following diffs
from master - although I'm not quite sure if they work later on:

For loader:

-            <ref name="absFilePath"/>
+            <optional>
+              <ref name="osLoaderNvramBacking"/>
+            </optional>
+            <ref name="osLoaderNvramSource"/>


For nvram:

-              <ref name="absFilePath"/>
+              <ref name="osLoaderNvramBacking"/>
+            </optional>
+            <optional>
+              <ref name="osLoaderNvramSource"/>


and just before <define name="ostypexen">

+
+  <define name="osLoaderNvramBacking">
+    <attribute name="backing">
+      <choice>
+        <value>file</value>
+        <value>network</value>
+      </choice>
+    </attribute>
+  </define>
+
+  <define name="osLoaderNvramSource">
+    <choice>
+      <group>
+        <ref name="absFilePath"/>
+        <empty/>
+      </group>
+      <group>
+        <ref name="diskSourceFileElement"/>
+        <ref name="diskSourceNetworkProtocolNBD"/>
+        <ref name="diskSourceNetworkProtocolGluster"/>
+        <ref name="diskSourceNetworkProtocolRBD"/>
+        <ref name="diskSourceNetworkProtocolISCSI"/>
+        <ref name="diskSourceNetworkProtocolHTTP"/>
+        <ref name="diskSourceNetworkProtocolSimple"/>
+      </group>
+    </choice>
+  </define>
+

But again - I'm not the expert... maybe someone else will have some
ideas/help in this area.

At the very least using the <define>'s in order to reduce the
cut-copy-paste for loader and nvram is a must. How exactly the grammar
has to look to make things work - that's TBD.

>              </optional>
>            </element>
>          </optional>
> @@ -1494,25 +1562,29 @@
>        </attribute>
>      </optional>
>      <optional>
> -      <element name="source">
> -        <optional>
> -          <attribute name="file">
> -            <ref name="absFilePath"/>
> -          </attribute>
> -        </optional>
> -        <optional>
> -          <ref name="storageStartupPolicy"/>
> -        </optional>
> -        <optional>
> -          <ref name="encryption"/>
> -        </optional>
> -        <zeroOrMore>
> -          <ref name='devSeclabel'/>
> -        </zeroOrMore>
> -      </element>
> +      <ref name='diskSourceFileElement'/>
>      </optional>
>    </define>
>  
> +  <define name="diskSourceFileElement">
> +    <element name="source">
> +      <optional>
> +        <attribute name="file">
> +          <ref name="absFilePath"/>
> +        </attribute>
> +      </optional>
> +      <optional>
> +        <ref name="storageStartupPolicy"/>
> +      </optional>
> +      <optional>
> +        <ref name="encryption"/>
> +      </optional>
> +      <zeroOrMore>
> +        <ref name='devSeclabel'/>
> +      </zeroOrMore>
> +    </element>
> +  </define>
> +

I would have extracted this patch into it's own patch, but as noted
above - I'm not convinced you need to have the <source> element using a
file attribute.

John

>    <define name="diskSourceBlock">
>      <attribute name="type">
>        <value>block</value>
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 01/12] Schema: Introduce XML schema for network-backed loader and nvram elements.
Posted by Prerna, 1 day ago
Hi John,
Thanks for the review.

On Thu, May 17, 2018 at 3:45 AM, John Ferlan <jferlan@redhat.com> wrote:

> $SUBJ:
>
> Is a bit long - goal is <= 70-ish characters
>

Agree, I'll fix this.


>
> On 05/14/2018 07:15 AM, Prerna Saxena wrote:
> > Today, 'loader' and 'nvram' elements are supposed to be backed by a
> local disk.
> > Given that NVRAM data could be network-backed for high availability,
> this patch
> > defines XML spec for serving loader & nvram disks via the network.
> >
> > Signed-off-by: Prerna Saxena <saxenap.ltc@gmail.com>
> > ---
> >  docs/schemas/domaincommon.rng | 108 ++++++++++++++++++++++++++++++
> +++++-------
> >  1 file changed, 90 insertions(+), 18 deletions(-)
> >
>
> First off: applying all the patches and running make w/ "check" and
> "syntax-check" fails during "check" w/ qemuxml2argvtest and
> qemuxml2xmltest failing.
>
>
I had thought make rpm ran both, but later found that it did not. Have the
next series ready which :
(1) combines all the related patches into ones that do not break the build.
(2) Pass make check and syntax-check.



> Before posting patches those must pass for each patch. When I get to
> patch 2, the build breaks. While one can appreciate having less to
> review in each patch, it's not possible to accept or review patches
> which cause a build break. Shouldn't be up to the reviewer to figure out
> how to make the series work. The rule is - each patch must be separately
> compileable and capable of passing check and syntax-check. If one ever
> needs to git bisect to determine where a problem lies, it'd be very
> awful if the build broke.
>
> As for this patch in particular, there are two things going on in this
> patch - #1 altering the <loader> and <nvram> schema and #2 extracting
> out part of the diskSourceFile to be used in #1.  Ironically, Thing 2 is
> creating a referenced name to be included in part of Thing 1; however,
> Thing 1 is essentially a cut-n-paste of the same thing. All those
> elements that are cut-n-pasted are part of diskSourceNetwork, except you
> didn't include VxHS in your list.
>

Did not include that since I was not sure if it is really useful.


>
> Still, a question in my mind is whether you really need to format the
> file using source. If the goal is to provide the ability to access
> networked storage, why provide the option to allow someone to change
> their XML from:
>
> <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader>
>
> to
>
> <loader type='rom' backing='file'>
>   <source file='/usr/lib/xen/boot/hvmloader'/>
> </loader>
>
> Yes, they are equivalent, but it seems the reason for it was because you
> wanted to format the former into the latter at one point in time.

If you limit to network only, then perhaps your optional attribute
> changes to be network='yes' which means to parse a <source> which may
> clear up some of the "odd" pieces of the grammar below.
>
>
Just accounting for network source alone using <source> and directly
specifying absolute paths
does not look like a clean design to me. When the <source> tag can describe
all storage sources,
we can unify the parsing and formatting of data using the helpers for
virStorageSource*.
If not, redundant code needs to be maintained for treating the two types
separately.

Please note that I am maintaining helpers to format-as-you-read, because
that seems to be a requirement.
But the underlying implementation should be able to unify code treating
these two formats as mere
rendering choices.


> > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.
> rng
> > index 0a6b29b..a6207db 100644
> > --- a/docs/schemas/domaincommon.rng
> > +++ b/docs/schemas/domaincommon.rng
> > @@ -276,7 +276,42 @@
> >                  </choice>
> >                </attribute>
> >              </optional>
> > -            <ref name="absFilePath"/>
> > +            <optional>
> > +              <attribute name="backing">
> > +                <choice>
> > +                  <value>file</value>
> > +                  <value>network</value>
> > +                </choice>
> > +              </attribute>
> > +            </optional>
> > +            <optional>
> > +              <choice>
> > +                 <group>
> > +                  <ref name="absFilePath"/>
> > +                 </group>
>
> This won't be equivalent to what you started with. Prior to this change
> absFilePath was required, now it would be optional.
>

absFilePath will become optional as there is now > 1 way to specify the
storage. This is an intended side effect of broadening the spec.



>
> I would assume that if it's not optional here for absFilePath, then the
> <source> cannot be optional as well.
>

A user can specify either absFilePath, or the description of the backing
source using the various diskSource* elements.
Individually, neither of them are mandatory, but at least one of those
needs to be provided.
The code takes care of this case but unfortunately I could not find a way
to specify "exactly one of.. " relation in the schema.


>
> > +                 <group>
> > +                  <ref name="diskSourceFileElement"/>
> > +                 </group>
> > +                 <group>
> > +                  <ref name="diskSourceNetworkProtocolNBD"/>
> > +                 </group>
> > +                 <group>
> > +                  <ref name="diskSourceNetworkProtocolGluster"/>
> > +                 </group>
> > +                 <group>
> > +                  <ref name="diskSourceNetworkProtocolRBD"/>
> > +                 </group>
> > +                 <group>
> > +                  <ref name="diskSourceNetworkProtocolISCSI"/>
> > +                 </group>
> > +                 <group>
> > +                  <ref name="diskSourceNetworkProtocolHTTP"/>
> > +                 </group>
> > +                 <group>
> > +                  <ref name="diskSourceNetworkProtocolSimple"/>
> > +                 </group>
> > +              </choice>
> > +            </optional>
> >            </element>
> >          </optional>
> >          <optional>
> > @@ -287,7 +322,40 @@
> >                </attribute>
> >              </optional>
> >              <optional>
> > -              <ref name="absFilePath"/>
> > +              <attribute name="backing">
> > +                <choice>
> > +                  <value>file</value>
> > +                  <value>network</value>
> > +                </choice>
> > +              </attribute>
> > +            </optional>
> > +            <optional>
> > +              <choice>
> > +                 <group>
> > +                  <ref name="absFilePath"/>
> > +                 </group>
>
> This is slightly different as absFilePath is optional...
>
> > +                 <group>
> > +                  <ref name="diskSourceFileElement"/>
> > +                 </group>
> > +                 <group>
> > +                  <ref name="diskSourceNetworkProtocolNBD"/>
> > +                 </group>
> > +                 <group>
> > +                  <ref name="diskSourceNetworkProtocolGluster"/>
> > +                 </group>
> > +                 <group>
> > +                  <ref name="diskSourceNetworkProtocolRBD"/>
> > +                 </group>
> > +                 <group>
> > +                  <ref name="diskSourceNetworkProtocolISCSI"/>
> > +                 </group>
> > +                 <group>
> > +                  <ref name="diskSourceNetworkProtocolHTTP"/>
> > +                 </group>
> > +                 <group>
> > +                  <ref name="diskSourceNetworkProtocolSimple"/>
> > +                 </group>
> > +              </choice>
>
>
>
> RNG format not my area of expertise - I usually copy, paste, and pray it
> works. Still the above looks really strange with all those <group> ..
> </group>... After a bit of playing I came up with the following diffs
> from master - although I'm not quite sure if they work later on:
>
> For loader:
>
> -            <ref name="absFilePath"/>
> +            <optional>
> +              <ref name="osLoaderNvramBacking"/>
> +            </optional>
> +            <ref name="osLoaderNvramSource"/>
>
>
> For nvram:
>
> -              <ref name="absFilePath"/>
> +              <ref name="osLoaderNvramBacking"/>
> +            </optional>
> +            <optional>
> +              <ref name="osLoaderNvramSource"/>
>
>
> and just before <define name="ostypexen">
>
> +
> +  <define name="osLoaderNvramBacking">
> +    <attribute name="backing">
> +      <choice>
> +        <value>file</value>
> +        <value>network</value>
> +      </choice>
> +    </attribute>
> +  </define>
> +
> +  <define name="osLoaderNvramSource">
> +    <choice>
> +      <group>
> +        <ref name="absFilePath"/>
> +        <empty/>
> +      </group>
> +      <group>
> +        <ref name="diskSourceFileElement"/>
> +        <ref name="diskSourceNetworkProtocolNBD"/>
> +        <ref name="diskSourceNetworkProtocolGluster"/>
> +        <ref name="diskSourceNetworkProtocolRBD"/>
> +        <ref name="diskSourceNetworkProtocolISCSI"/>
> +        <ref name="diskSourceNetworkProtocolHTTP"/>
> +        <ref name="diskSourceNetworkProtocolSimple"/>
> +      </group>
> +    </choice>
> +  </define>
> +
>
> But again - I'm not the expert... maybe someone else will have some
> ideas/help in this area.
>
> At the very least using the <define>'s in order to reduce the
> cut-copy-paste for loader and nvram is a must. How exactly the grammar
> has to look to make things work - that's TBD.
>
> >              </optional>
> >            </element>
> >          </optional>
> > @@ -1494,25 +1562,29 @@
> >        </attribute>
> >      </optional>
> >      <optional>
> > -      <element name="source">
> > -        <optional>
> > -          <attribute name="file">
> > -            <ref name="absFilePath"/>
> > -          </attribute>
> > -        </optional>
> > -        <optional>
> > -          <ref name="storageStartupPolicy"/>
> > -        </optional>
> > -        <optional>
> > -          <ref name="encryption"/>
> > -        </optional>
> > -        <zeroOrMore>
> > -          <ref name='devSeclabel'/>
> > -        </zeroOrMore>
> > -      </element>
> > +      <ref name='diskSourceFileElement'/>
> >      </optional>
> >    </define>
> >
> > +  <define name="diskSourceFileElement">
> > +    <element name="source">
> > +      <optional>
> > +        <attribute name="file">
> > +          <ref name="absFilePath"/>
> > +        </attribute>
> > +      </optional>
> > +      <optional>
> > +        <ref name="storageStartupPolicy"/>
> > +      </optional>
> > +      <optional>
> > +        <ref name="encryption"/>
> > +      </optional>
> > +      <zeroOrMore>
> > +        <ref name='devSeclabel'/>
> > +      </zeroOrMore>
> > +    </element>
> > +  </define>
> > +
>
> I would have extracted this patch into it's own patch, but as noted
> above - I'm not convinced you need to have the <source> element using a
> file attribute.
>
>
I believe we need that for consistency's sake. I do not see any benefit of
using "source" for just network elements alone, and directly specifying
file paths the old way. I agree it seems like a less invasive way, but it
makes both the code and documentation very counter-intuitive.

My next set of patches which pass unit tests and syntax-check are ready,
and I think I will post them to the list.
Maybe it is easier to discuss the XML semantics over a saner patchqueue.

Thanks,
Prerna
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 02/12] Loader: Add a more elaborate definition.
Posted by Prerna Saxena, 1 week ago
Augment definition to include virStorageSourcePtr that
more comprehensively describes the nature of backing element.
Also include flags for annotating if input XML definition is
old-style or new-style.

Signed-off-by: Prerna Saxena <saxenap.ltc@gmail.com>
---
 src/conf/domain_conf.h | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 15d228b..bbd021b 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1855,15 +1855,22 @@ typedef enum {
 
 VIR_ENUM_DECL(virDomainLoader)
 
+struct _virStorageSource;
+typedef struct _virStorageSource* virStorageSourcePtr;
+
 typedef struct _virDomainLoaderDef virDomainLoaderDef;
 typedef virDomainLoaderDef *virDomainLoaderDefPtr;
 struct _virDomainLoaderDef {
-    char *path;
+    virStorageSourcePtr src;
     int readonly;   /* enum virTristateBool */
     virDomainLoader type;
     int secure;     /* enum virTristateBool */
-    char *nvram;    /* path to non-volatile RAM */
+    virStorageSourcePtr nvram;  /* path to non-voliatile RAM */
     char *templt;   /* user override of path to master nvram */
+    bool oldStyleLoader;  /* is this an old-style XML formatting,
+                           * ie, absolute path is directly specified? */
+    bool oldStyleNvram;   /* is this an old-style XML formatting,
+                           * ie, absolute path is directly specified? */
 };
 
 void virDomainLoaderDefFree(virDomainLoaderDefPtr loader);
-- 
1.8.1.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 03/12] Parse domain XML to generate virDomainLoaderDef & virDomainNvramDef.
Posted by Prerna Saxena, 1 week ago
This patch is used to interpret domain XML and store the Loader &
Nvram's backing definitions as virStorageSource.
It also identifies if input XML used old or new-style declaration.
(This will later be used by the formatter).

Signed-off-by: Prerna Saxena <saxenap.ltc@gmail.com>
---
 src/conf/domain_conf.c | 131 ++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 124 insertions(+), 7 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f678e26..be43695 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2884,8 +2884,8 @@ virDomainLoaderDefFree(virDomainLoaderDefPtr loader)
     if (!loader)
         return;
 
-    VIR_FREE(loader->path);
-    VIR_FREE(loader->nvram);
+    virStorageSourceFree(loader->src);
+    virStorageSourceFree(loader->nvram);
     VIR_FREE(loader->templt);
     VIR_FREE(loader);
 }
@@ -17986,17 +17986,62 @@ virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def)
 
 static int
 virDomainLoaderDefParseXML(xmlNodePtr node,
+                           xmlXPathContextPtr ctxt,
                            virDomainLoaderDefPtr loader)
 {
     int ret = -1;
     char *readonly_str = NULL;
     char *secure_str = NULL;
     char *type_str = NULL;
+    char *tmp = NULL;
+    xmlNodePtr cur;
+    xmlXPathContextPtr cur_ctxt = ctxt;
+
+    if (VIR_ALLOC(loader->src)) {
+        goto cleanup;
+    }
+    loader->src->type = VIR_STORAGE_TYPE_LAST;
+    loader->oldStyleLoader = false;
 
     readonly_str = virXMLPropString(node, "readonly");
     secure_str = virXMLPropString(node, "secure");
     type_str = virXMLPropString(node, "type");
-    loader->path = (char *) xmlNodeGetContent(node);
+
+    if ((tmp = virXMLPropString(node, "backing")) &&
+        (loader->src->type = virStorageTypeFromString(tmp)) <= 0) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                       _("unknown loader type '%s'"), tmp);
+        goto cleanup;
+    }
+    VIR_FREE(tmp);
+
+    for (cur = node->children; cur != NULL; cur = cur->next) {
+        if (cur->type != XML_ELEMENT_NODE) {
+            continue;
+        }
+
+        if (virXMLNodeNameEqual(cur, "source")) {
+            /* new-style declaration found */
+            if (virDomainStorageSourceParse(cur, cur_ctxt, loader->src, 0) < 0) {
+                virReportError(VIR_ERR_XML_DETAIL,
+                               _("Error parsing Loader source element"));
+                goto cleanup;
+            }
+            break;
+        }
+    }
+
+    /* Old-style absolute path found ? */
+    if (loader->src->type == VIR_STORAGE_TYPE_LAST) {
+        if (!(loader->src->path = (char *) xmlNodeGetContent(node))) {
+            virReportError(VIR_ERR_XML_ERROR, "%s",
+                           _("missing loader source"));
+            goto cleanup;
+        } else {
+            loader->src->type = VIR_STORAGE_TYPE_FILE;
+            loader->oldStyleLoader = true;
+        }
+    }
 
     if (readonly_str &&
         (loader->readonly = virTristateBoolTypeFromString(readonly_str)) <= 0) {
@@ -18023,13 +18068,81 @@ virDomainLoaderDefParseXML(xmlNodePtr node,
     }
 
     ret = 0;
- cleanup:
+    goto exit;
+cleanup:
+    if (loader->src)
+      VIR_FREE(loader->src);
+exit:
     VIR_FREE(readonly_str);
     VIR_FREE(secure_str);
     VIR_FREE(type_str);
+
     return ret;
 }
 
+static int
+virDomainLoaderNvramDefParseXML(xmlNodePtr node,
+                           xmlXPathContextPtr ctxt,
+                           virDomainLoaderDefPtr loader)
+{
+    int ret = -1;
+    char *tmp = NULL;
+    xmlNodePtr cur;
+
+    if (VIR_ALLOC(loader->nvram)) {
+        goto cleanup;
+    }
+
+    loader->nvram->type = VIR_STORAGE_TYPE_LAST;
+    loader->nvram->oldStyleNvram = false;
+
+    if ((tmp = virXMLPropString(node, "backing")) &&
+        (loader->nvram->type = virStorageTypeFromString(tmp)) <= 0) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                       _("unknown nvram type '%s'"), tmp);
+        goto cleanup;
+    }
+    VIR_FREE(tmp);
+
+    for (cur = node->children; cur != NULL; cur = cur->next) {
+        if (cur->type != XML_ELEMENT_NODE) {
+            continue;
+        }
+
+        if (virXMLNodeNameEqual(cur, "template")) {
+            loader->templt = virXPathString("string(./os/nvram[1]/@template)", ctxt);
+            continue;
+        }
+
+        if (virXMLNodeNameEqual(cur, "source")) {
+            if (virDomainStorageSourceParse(cur, ctxt, loader->nvram, 0) < 0) {
+                virReportError(VIR_ERR_XML_DETAIL,
+                               _("Error parsing nvram source element"));
+                goto cleanup;
+            }
+            ret = 0;
+        }
+    }
+
+    /* Old-style declaration found */
+    if (loader->nvram->type == VIR_STORAGE_TYPE_LAST) {
+        if (!(loader->nvram->path = (char *) xmlNodeGetContent(node))) {
+            virReportError(VIR_ERR_XML_ERROR, "%s",
+                           _("missing nvram source"));
+            goto cleanup;
+        } else {
+            loader->nvram->type = VIR_STORAGE_TYPE_FILE;
+            loader->oldStyleNvram = true;
+            ret = 0;
+        }
+    }
+    return ret;
+
+cleanup:
+    if (loader->nvram)
+      VIR_FREE(loader->nvram);
+    return ret;
+}
 
 static virBitmapPtr
 virDomainSchedulerParse(xmlNodePtr node,
@@ -18422,11 +18535,15 @@ virDomainDefParseBootOptions(virDomainDefPtr def,
             if (VIR_ALLOC(def->os.loader) < 0)
                 goto error;
 
-            if (virDomainLoaderDefParseXML(loader_node, def->os.loader) < 0)
+            def->os.loader->src = NULL;
+            def->os.loader->nvram = NULL;
+            if (virDomainLoaderDefParseXML(loader_node, ctxt, def->os.loader) < 0)
                 goto error;
 
-            def->os.loader->nvram = virXPathString("string(./os/nvram[1])", ctxt);
-            def->os.loader->templt = virXPathString("string(./os/nvram[1]/@template)", ctxt);
+            if ((loader_node = virXPathNode("./os/nvram[1]", ctxt)) &&
+                (virDomainLoaderNvramDefParseXML(loader_node, ctxt,
+                                                 def->os.loader) < 0))
+                    goto error;
         }
     }
 
-- 
1.8.1.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 03/12] Parse domain XML to generate virDomainLoaderDef & virDomainNvramDef.
Posted by Marc Hartmayer, 1 week ago
On Mon, May 14, 2018 at 01:15 PM +0200, Prerna Saxena <saxenap.ltc@gmail.com> wrote:
> This patch is used to interpret domain XML and store the Loader &
> Nvram's backing definitions as virStorageSource.
> It also identifies if input XML used old or new-style declaration.
> (This will later be used by the formatter).
>
> Signed-off-by: Prerna Saxena <saxenap.ltc@gmail.com>
> ---
>  src/conf/domain_conf.c | 131 ++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 124 insertions(+), 7 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index f678e26..be43695 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2884,8 +2884,8 @@ virDomainLoaderDefFree(virDomainLoaderDefPtr loader)
>      if (!loader)
>          return;
>
> -    VIR_FREE(loader->path);
> -    VIR_FREE(loader->nvram);
> +    virStorageSourceFree(loader->src);
> +    virStorageSourceFree(loader->nvram);
>      VIR_FREE(loader->templt);
>      VIR_FREE(loader);
>  }
> @@ -17986,17 +17986,62 @@ virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def)
>
>  static int
>  virDomainLoaderDefParseXML(xmlNodePtr node,
> +                           xmlXPathContextPtr ctxt,
>                             virDomainLoaderDefPtr loader)
>  {
>      int ret = -1;
>      char *readonly_str = NULL;
>      char *secure_str = NULL;
>      char *type_str = NULL;
> +    char *tmp = NULL;
> +    xmlNodePtr cur;
> +    xmlXPathContextPtr cur_ctxt = ctxt;
> +
> +    if (VIR_ALLOC(loader->src)) {
                                 ^^
                                 Usually it is checked for < 0.

> +        goto cleanup;
> +    }
> +    loader->src->type = VIR_STORAGE_TYPE_LAST;
> +    loader->oldStyleLoader = false;
>
>      readonly_str = virXMLPropString(node, "readonly");
>      secure_str = virXMLPropString(node, "secure");
>      type_str = virXMLPropString(node, "type");
> -    loader->path = (char *) xmlNodeGetContent(node);
> +
> +    if ((tmp = virXMLPropString(node, "backing")) &&
> +        (loader->src->type = virStorageTypeFromString(tmp)) <= 0) {

If virStorageTypeFromString fails there is a memory leak of @tmp.

> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("unknown loader type '%s'"), tmp);
> +        goto cleanup;
> +    }
> +    VIR_FREE(tmp);
> +
> +    for (cur = node->children; cur != NULL; cur = cur->next) {
> +        if (cur->type != XML_ELEMENT_NODE) {
> +            continue;
> +        }
> +
> +        if (virXMLNodeNameEqual(cur, "source")) {
> +            /* new-style declaration found */
> +            if (virDomainStorageSourceParse(cur, cur_ctxt, loader->src, 0) < 0) {
> +                virReportError(VIR_ERR_XML_DETAIL,
> +                               _("Error parsing Loader source element"));
> +                goto cleanup;
> +            }
> +            break;
> +        }
> +    }
> +
> +    /* Old-style absolute path found ? */
> +    if (loader->src->type == VIR_STORAGE_TYPE_LAST) {
> +        if (!(loader->src->path = (char *) xmlNodeGetContent(node))) {
> +            virReportError(VIR_ERR_XML_ERROR, "%s",
> +                           _("missing loader source"));
> +            goto cleanup;
> +        } else {
> +            loader->src->type = VIR_STORAGE_TYPE_FILE;
> +            loader->oldStyleLoader = true;
> +        }
> +    }
>
>      if (readonly_str &&
>          (loader->readonly = virTristateBoolTypeFromString(readonly_str)) <= 0) {
> @@ -18023,13 +18068,81 @@ virDomainLoaderDefParseXML(xmlNodePtr node,
>      }
>
>      ret = 0;
> - cleanup:
> +    goto exit;
> +cleanup:

I would replace: s/cleanup/error and s/exit/cleanup.

> +    if (loader->src)
> +      VIR_FREE(loader->src);

Shouldn’t it be a virStorageSourceFree() here? Not sure if it’s needed
but it’s safer.

> +exit:
>      VIR_FREE(readonly_str);
>      VIR_FREE(secure_str);
>      VIR_FREE(type_str);
> +
>      return ret;
>  }
>
> +static int
> +virDomainLoaderNvramDefParseXML(xmlNodePtr node,
> +                           xmlXPathContextPtr ctxt,
> +                           virDomainLoaderDefPtr loader)
> +{
> +    int ret = -1;
> +    char *tmp = NULL;
> +    xmlNodePtr cur;
> +
> +    if (VIR_ALLOC(loader->nvram)) {
                                   ^^
                                   Usually it is checked for < 0.


> +        goto cleanup;
> +    }

Unneeded braces.

> +
> +    loader->nvram->type = VIR_STORAGE_TYPE_LAST;
> +    loader->nvram->oldStyleNvram = false;
> +
> +    if ((tmp = virXMLPropString(node, "backing")) &&
> +        (loader->nvram->type = virStorageTypeFromString(tmp)) <= 0) {

If virStorageTypeFromString fails there is a memory leak of @tmp.

> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("unknown nvram type '%s'"), tmp);
> +        goto cleanup;
> +    }
> +    VIR_FREE(tmp);
> +
> +    for (cur = node->children; cur != NULL; cur = cur->next) {
> +        if (cur->type != XML_ELEMENT_NODE) {
> +            continue;
> +        }
> +
> +        if (virXMLNodeNameEqual(cur, "template")) {
> +            loader->templt = virXPathString("string(./os/nvram[1]/@template)", ctxt);

Shouldn’t this be cleaned up in case of an error?

> +            continue;
> +        }
> +
> +        if (virXMLNodeNameEqual(cur, "source")) {
> +            if (virDomainStorageSourceParse(cur, ctxt, loader->nvram, 0) < 0) {
> +                virReportError(VIR_ERR_XML_DETAIL,
> +                               _("Error parsing nvram source element"));
> +                goto cleanup;
> +            }
> +            ret = 0;
> +        }
> +    }
> +
> +    /* Old-style declaration found */
> +    if (loader->nvram->type == VIR_STORAGE_TYPE_LAST) {
> +        if (!(loader->nvram->path = (char *) xmlNodeGetContent(node))) {
> +            virReportError(VIR_ERR_XML_ERROR, "%s",
> +                           _("missing nvram source"));
> +            goto cleanup;
> +        } else {
> +            loader->nvram->type = VIR_STORAGE_TYPE_FILE;
> +            loader->oldStyleNvram = true;
> +            ret = 0;
> +        }
> +    }
> +    return ret;
> +
> +cleanup:
> +    if (loader->nvram)
> +      VIR_FREE(loader->nvram);

virStorageSourceFree…

> +    return ret;
> +}
>
>  static virBitmapPtr
>  virDomainSchedulerParse(xmlNodePtr node,
> @@ -18422,11 +18535,15 @@ virDomainDefParseBootOptions(virDomainDefPtr def,
>              if (VIR_ALLOC(def->os.loader) < 0)
>                  goto error;
>
> -            if (virDomainLoaderDefParseXML(loader_node, def->os.loader) < 0)
> +            def->os.loader->src = NULL;
> +            def->os.loader->nvram = NULL;

Should be unneeded as VIR_ALLOC used calloc.

> +            if (virDomainLoaderDefParseXML(loader_node, ctxt, def->os.loader) < 0)
>                  goto error;
>
> -            def->os.loader->nvram = virXPathString("string(./os/nvram[1])", ctxt);
> -            def->os.loader->templt = virXPathString("string(./os/nvram[1]/@template)", ctxt);
> +            if ((loader_node = virXPathNode("./os/nvram[1]", ctxt)) &&
> +                (virDomainLoaderNvramDefParseXML(loader_node, ctxt,
> +                                                 def->os.loader) < 0))
> +                    goto error;
>          }
>      }

Note: I didn't take a closer look.

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

Beste Grüße / Kind regards
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 03/12] Parse domain XML to generate virDomainLoaderDef & virDomainNvramDef.
Posted by Peter Krempa, 1 week ago
On Mon, May 14, 2018 at 16:20:53 +0200, Marc Hartmayer wrote:
> On Mon, May 14, 2018 at 01:15 PM +0200, Prerna Saxena <saxenap.ltc@gmail.com> wrote:
> > This patch is used to interpret domain XML and store the Loader &
> > Nvram's backing definitions as virStorageSource.
> > It also identifies if input XML used old or new-style declaration.
> > (This will later be used by the formatter).
> >
> > Signed-off-by: Prerna Saxena <saxenap.ltc@gmail.com>
> > ---
> >  src/conf/domain_conf.c | 131 ++++++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 124 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index f678e26..be43695 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -2884,8 +2884,8 @@ virDomainLoaderDefFree(virDomainLoaderDefPtr loader)
> >      if (!loader)
> >          return;
> >
> > -    VIR_FREE(loader->path);
> > -    VIR_FREE(loader->nvram);
> > +    virStorageSourceFree(loader->src);
> > +    virStorageSourceFree(loader->nvram);
> >      VIR_FREE(loader->templt);
> >      VIR_FREE(loader);
> >  }
> > @@ -17986,17 +17986,62 @@ virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def)
> >
> >  static int
> >  virDomainLoaderDefParseXML(xmlNodePtr node,
> > +                           xmlXPathContextPtr ctxt,
> >                             virDomainLoaderDefPtr loader)
> >  {
> >      int ret = -1;
> >      char *readonly_str = NULL;
> >      char *secure_str = NULL;
> >      char *type_str = NULL;
> > +    char *tmp = NULL;
> > +    xmlNodePtr cur;
> > +    xmlXPathContextPtr cur_ctxt = ctxt;
> > +
> > +    if (VIR_ALLOC(loader->src)) {
>                                  ^^
>                                  Usually it is checked for < 0.
> 
> > +        goto cleanup;
> > +    }
> > +    loader->src->type = VIR_STORAGE_TYPE_LAST;
> > +    loader->oldStyleLoader = false;
> >
> >      readonly_str = virXMLPropString(node, "readonly");
> >      secure_str = virXMLPropString(node, "secure");
> >      type_str = virXMLPropString(node, "type");
> > -    loader->path = (char *) xmlNodeGetContent(node);
> > +
> > +    if ((tmp = virXMLPropString(node, "backing")) &&
> > +        (loader->src->type = virStorageTypeFromString(tmp)) <= 0) {
> 
> If virStorageTypeFromString fails there is a memory leak of @tmp.
> 
> > +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +                       _("unknown loader type '%s'"), tmp);
> > +        goto cleanup;
> > +    }
> > +    VIR_FREE(tmp);
> > +
> > +    for (cur = node->children; cur != NULL; cur = cur->next) {
> > +        if (cur->type != XML_ELEMENT_NODE) {
> > +            continue;
> > +        }
> > +
> > +        if (virXMLNodeNameEqual(cur, "source")) {
> > +            /* new-style declaration found */
> > +            if (virDomainStorageSourceParse(cur, cur_ctxt, loader->src, 0) < 0) {
> > +                virReportError(VIR_ERR_XML_DETAIL,
> > +                               _("Error parsing Loader source element"));
> > +                goto cleanup;
> > +            }
> > +            break;
> > +        }
> > +    }
> > +
> > +    /* Old-style absolute path found ? */
> > +    if (loader->src->type == VIR_STORAGE_TYPE_LAST) {
> > +        if (!(loader->src->path = (char *) xmlNodeGetContent(node))) {
> > +            virReportError(VIR_ERR_XML_ERROR, "%s",
> > +                           _("missing loader source"));
> > +            goto cleanup;
> > +        } else {
> > +            loader->src->type = VIR_STORAGE_TYPE_FILE;
> > +            loader->oldStyleLoader = true;
> > +        }
> > +    }
> >
> >      if (readonly_str &&
> >          (loader->readonly = virTristateBoolTypeFromString(readonly_str)) <= 0) {
> > @@ -18023,13 +18068,81 @@ virDomainLoaderDefParseXML(xmlNodePtr node,
> >      }
> >
> >      ret = 0;
> > - cleanup:
> > +    goto exit;
> > +cleanup:
> 
> I would replace: s/cleanup/error and s/exit/cleanup.
> 
> > +    if (loader->src)
> > +      VIR_FREE(loader->src);
> 
> Shouldn’t it be a virStorageSourceFree() here? Not sure if it’s needed
> but it’s safer.

Additionally the preferred way is to use a second virStorageSourcePtr to
hold the data while it's being parsed and then use VIR_STEAL_PTR to put
it into the structure.

The cleanup section then can call virStorageSourceFree on the temp ptr
unconditionally and it also avoids this messy labels.

> 
> > +exit:
> >      VIR_FREE(readonly_str);
> >      VIR_FREE(secure_str);
> >      VIR_FREE(type_str);
> > +
> >      return ret;
> >  }
> >
> > +static int
> > +virDomainLoaderNvramDefParseXML(xmlNodePtr node,
> > +                           xmlXPathContextPtr ctxt,
> > +                           virDomainLoaderDefPtr loader)
> > +{
> > +    int ret = -1;
> > +    char *tmp = NULL;
> > +    xmlNodePtr cur;
> > +
> > +    if (VIR_ALLOC(loader->nvram)) {
>                                    ^^
>                                    Usually it is checked for < 0.
> 
> 
> > +        goto cleanup;
> > +    }
> 
> Unneeded braces.

Actually these would fail the syntax-check. Please make sure that
syntax-check passes on every patch.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 04/12] Format the loader source appropriately.
Posted by Prerna Saxena, 1 week ago
If the initial XML used the old-style declaration as follows:
<loader type='pflash'>/path/to/file</loader>

we format it as was read.

However, if it used new-style declaration:
<loader type='pflash' backing='file'>
  <source file='path/to/file'/>
</loader>

The formatter identifies that this is a new-style format
and renders it likewise.

Signed-off-by: Prerna Saxena <saxenap.ltc@gmail.com>
---
 src/conf/domain_conf.c | 86 ++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 76 insertions(+), 10 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index be43695..d59a579 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -18094,7 +18094,7 @@ virDomainLoaderNvramDefParseXML(xmlNodePtr node,
     }
 
     loader->nvram->type = VIR_STORAGE_TYPE_LAST;
-    loader->nvram->oldStyleNvram = false;
+    loader->oldStyleNvram = false;
 
     if ((tmp = virXMLPropString(node, "backing")) &&
         (loader->nvram->type = virStorageTypeFromString(tmp)) <= 0) {
@@ -26212,11 +26212,19 @@ virDomainHugepagesFormat(virBufferPtr buf,
 
 static void
 virDomainLoaderDefFormat(virBufferPtr buf,
-                         virDomainLoaderDefPtr loader)
+                         virDomainLoaderDefPtr loader,
+                         unsigned int flags)
 {
     const char *readonly = virTristateBoolTypeToString(loader->readonly);
     const char *secure = virTristateBoolTypeToString(loader->secure);
     const char *type = virDomainLoaderTypeToString(loader->type);
+    const char *backing = NULL;
+
+    virBuffer attrBuf = VIR_BUFFER_INITIALIZER;
+    virBuffer childBuf = VIR_BUFFER_INITIALIZER;
+
+    virBufferSetChildIndent(&childBuf, buf);
+
 
     virBufferAddLit(buf, "<loader");
 
@@ -26226,17 +26234,75 @@ virDomainLoaderDefFormat(virBufferPtr buf,
     if (loader->secure)
         virBufferAsprintf(buf, " secure='%s'", secure);
 
-    virBufferAsprintf(buf, " type='%s'>", type);
+    virBufferAsprintf(buf, " type='%s'", type);
+    if (loader->src &&
+        loader->src->type < VIR_STORAGE_TYPE_LAST) {
+        if (!loader->oldStyleLoader) {
+            /* Format this in the new style, using the
+             * <source> sub-element */
+            if (virDomainStorageSourceFormat(&attrBuf, &childBuf, loader->src,
+                                             flags, 0) < 0) {
+                    virReportError(VIR_ERR_INTERNAL_ERROR,
+                                 _("Cannot format loader source"));
+                    goto cleanup;
+            }
+
+            backing = virStorageTypeToString(loader->src->type);
+            virBufferAsprintf(buf, " backing='%s'>", backing);
+
+            if (virXMLFormatElement(buf, "source", &attrBuf, &childBuf) < 0) {
+                virReportError(VIR_ERR_INTERNAL_ERROR,
+                               _("Cannot format loader source"));
+                goto cleanup;
+            }
+
+        } else
+            /* Format this in the old-style, using absolute paths directly. */
+            virBufferAsprintf(buf, ">%s", loader->src->path);
+    } else
+        virBufferAddLit(buf, ">\n");
+
+    virBufferAddLit(buf, "</loader>\n");
 
-    virBufferEscapeString(buf, "%s</loader>\n", loader->path);
     if (loader->nvram || loader->templt) {
+        ignore_value(virBufferContentAndReset(&attrBuf));
+        ignore_value(virBufferContentAndReset(&childBuf));
+        virBufferSetChildIndent(&childBuf, buf);
+
         virBufferAddLit(buf, "<nvram");
-        virBufferEscapeString(buf, " template='%s'", loader->templt);
-        if (loader->nvram)
-            virBufferEscapeString(buf, ">%s</nvram>\n", loader->nvram);
-        else
-            virBufferAddLit(buf, "/>\n");
+
+        if (loader->templt)
+            virBufferEscapeString(buf, " template='%s'", loader->templt);
+
+        if (loader->nvram) {
+            backing = virStorageTypeToString(loader->nvram->type);
+            if (!loader->oldStyleNvram) {
+                if (virDomainStorageSourceFormat(&attrBuf, &childBuf,
+                                             loader->nvram, flags, 0) < 0) {
+                    virReportError(VIR_ERR_INTERNAL_ERROR,
+                                 _("Cannot format NVRAM source"));
+                    virBufferAddLit(buf, ">\n</nvram>\n");
+                    goto cleanup;
+                }
+
+                virBufferEscapeString(buf, " backing='%s'>", backing);
+                if (virXMLFormatElement(buf, "source", &attrBuf, &childBuf) < 0) {
+                    virReportError(VIR_ERR_INTERNAL_ERROR,
+                                 _("Cannot format NVRAM source"));
+                    virBufferAddLit(buf, "</nvram>\n");
+                    goto cleanup;
+                }
+            } else {
+                /* old-style NVRAM declaration found */
+                virBufferAsprintf(buf, ">%s", loader->nvram->path);
+            }
+            virBufferAddLit(buf, "\n</nvram>\n");
+        }
     }
+cleanup:
+    virBufferFreeAndReset(&attrBuf);
+    virBufferFreeAndReset(&childBuf);
+    return;
 }
 
 static void
@@ -26899,7 +26965,7 @@ virDomainDefFormatInternal(virDomainDefPtr def,
         virBufferAsprintf(buf, "<initgroup>%s</initgroup>\n", def->os.initgroup);
 
     if (def->os.loader)
-        virDomainLoaderDefFormat(buf, def->os.loader);
+        virDomainLoaderDefFormat(buf, def->os.loader, flags);
     virBufferEscapeString(buf, "<kernel>%s</kernel>\n",
                           def->os.kernel);
     virBufferEscapeString(buf, "<initrd>%s</initrd>\n",
-- 
1.8.1.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 05/12] Plumb the loader source into generation of QEMU command line.
Posted by Prerna Saxena, 1 week ago
Given that nvram & loader elements can now be backed by a non-local
source too, adjust all steps leading to generation of
QEMU command line.

Signed-off-by: Prerna Saxena <saxenap.ltc@gmail.com>
---
 src/qemu/qemu_cgroup.c          | 13 +++++++++----
 src/qemu/qemu_command.c         | 21 ++++++++++++++++-----
 src/qemu/qemu_domain.c          | 31 +++++++++++++++++++++---------
 src/qemu/qemu_driver.c          |  7 ++++---
 src/qemu/qemu_process.c         | 42 ++++++++++++++++++++++++++++-------------
 src/security/security_dac.c     |  6 ++++--
 src/security/security_selinux.c |  6 ++++--
 7 files changed, 88 insertions(+), 38 deletions(-)

diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index d88eb78..2068eb0 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -580,16 +580,21 @@ qemuSetupMemoryCgroup(virDomainObjPtr vm)
 static int
 qemuSetupFirmwareCgroup(virDomainObjPtr vm)
 {
+    virStorageSourcePtr src = NULL;
+
     if (!vm->def->os.loader)
         return 0;
 
-    if (vm->def->os.loader->path &&
-        qemuSetupImagePathCgroup(vm, vm->def->os.loader->path,
-                                 vm->def->os.loader->readonly == VIR_TRISTATE_BOOL_YES) < 0)
+    src = vm->def->os.loader->src;
+
+    if (src->type == VIR_STORAGE_TYPE_FILE &&
+        qemuSetupImagePathCgroup(vm, src->path,
+                                 src->readonly == VIR_TRISTATE_BOOL_YES) < 0)
         return -1;
 
     if (vm->def->os.loader->nvram &&
-        qemuSetupImagePathCgroup(vm, vm->def->os.loader->nvram, false) < 0)
+        vm->def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE &&
+        qemuSetupImagePathCgroup(vm, vm->def->os.loader->nvram->path, false) < 0)
         return -1;
 
     return 0;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 08f67a4..e9d6e4b 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9260,6 +9260,7 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr cmd,
     virDomainLoaderDefPtr loader = def->os.loader;
     virBuffer buf = VIR_BUFFER_INITIALIZER;
     int unit = 0;
+    char *source = NULL;
 
     if (!loader)
         return;
@@ -9267,7 +9268,7 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr cmd,
     switch ((virDomainLoader) loader->type) {
     case VIR_DOMAIN_LOADER_TYPE_ROM:
         virCommandAddArg(cmd, "-bios");
-        virCommandAddArg(cmd, loader->path);
+        virCommandAddArg(cmd, loader->src->path);
         break;
 
     case VIR_DOMAIN_LOADER_TYPE_PFLASH:
@@ -9279,9 +9280,14 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr cmd,
                                  NULL);
         }
 
+        if (qemuGetDriveSourceString(loader->src, NULL, &source) < 0)
+            break;
+
         virBufferAddLit(&buf, "file=");
-        virQEMUBuildBufferEscapeComma(&buf, loader->path);
-        virBufferAsprintf(&buf, ",if=pflash,format=raw,unit=%d", unit);
+        virQEMUBuildBufferEscapeComma(&buf, source);
+        free(source);
+        virBufferAsprintf(&buf, ",if=pflash,format=raw,unit=%d",
+                          unit);
         unit++;
 
         if (loader->readonly) {
@@ -9294,9 +9300,14 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr cmd,
 
         if (loader->nvram) {
             virBufferFreeAndReset(&buf);
+            if (qemuGetDriveSourceString(loader->nvram, NULL, &source) < 0)
+                break;
+
             virBufferAddLit(&buf, "file=");
-            virQEMUBuildBufferEscapeComma(&buf, loader->nvram);
-            virBufferAsprintf(&buf, ",if=pflash,format=raw,unit=%d", unit);
+            virQEMUBuildBufferEscapeComma(&buf, source);
+            virBufferAsprintf(&buf, ",if=pflash,format=raw,unit=%d",
+                              unit);
+            unit++;
 
             virCommandAddArg(cmd, "-drive");
             virCommandAddArgBuffer(cmd, &buf);
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 9bb6d8a..2d4e299 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3318,6 +3318,9 @@ qemuDomainDefPostParse(virDomainDefPtr def,
      * function shall not fail in that case. It will be re-run on VM startup
      * with the capabilities populated. */
     virQEMUCapsPtr qemuCaps = parseOpaque;
+    virDomainLoaderDefPtr ldr = NULL;
+    char *nvramPath = NULL;
+
     int ret = -1;
 
     if (def->os.bootloader || def->os.bootloaderArgs) {
@@ -3332,13 +3335,20 @@ qemuDomainDefPostParse(virDomainDefPtr def,
         goto cleanup;
     }
 
-    if (def->os.loader &&
-        def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_PFLASH &&
-        def->os.loader->readonly == VIR_TRISTATE_SWITCH_ON &&
-        !def->os.loader->nvram) {
-        if (virAsprintf(&def->os.loader->nvram, "%s/%s_VARS.fd",
+    ldr = def->os.loader;
+    if (ldr &&
+        ldr->type == VIR_DOMAIN_LOADER_TYPE_PFLASH &&
+        ldr->readonly == VIR_TRISTATE_SWITCH_ON &&
+        !ldr->nvram) {
+        if (virAsprintf(&nvramPath, "%s/%s_VARS.fd",
                         cfg->nvramDir, def->name) < 0)
             goto cleanup;
+        ldr->nvram = virStorageSourceNewFromBackingAbsolute(nvramPath);
+        if (!ldr->nvram) {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           _("Unable to add NVRAM drive %s"), nvramPath);
+            goto cleanup;
+        }
     }
 
     if (qemuDomainDefAddDefaultDevices(def, qemuCaps) < 0)
@@ -10494,19 +10504,22 @@ qemuDomainSetupLoader(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED,
 
     VIR_DEBUG("Setting up loader");
 
-    if (loader) {
+    if (loader && loader->src) {
         switch ((virDomainLoader) loader->type) {
         case VIR_DOMAIN_LOADER_TYPE_ROM:
-            if (qemuDomainCreateDevice(loader->path, data, false) < 0)
+            if (loader->src->type == VIR_STORAGE_TYPE_FILE &&
+                qemuDomainCreateDevice(loader->src->path, data, false) < 0)
                 goto cleanup;
             break;
 
         case VIR_DOMAIN_LOADER_TYPE_PFLASH:
-            if (qemuDomainCreateDevice(loader->path, data, false) < 0)
+            if (loader->src->type == VIR_STORAGE_TYPE_FILE &&
+                qemuDomainCreateDevice(loader->src->path, data, false) < 0)
                 goto cleanup;
 
             if (loader->nvram &&
-                qemuDomainCreateDevice(loader->nvram, data, false) < 0)
+                loader->nvram->type == VIR_STORAGE_TYPE_FILE &&
+                qemuDomainCreateDevice(loader->nvram->path, data, false) < 0)
                 goto cleanup;
             break;
 
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index c129321..9c491b2 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7540,12 +7540,13 @@ qemuDomainUndefineFlags(virDomainPtr dom,
 
     if (vm->def->os.loader &&
         vm->def->os.loader->nvram &&
-        virFileExists(vm->def->os.loader->nvram)) {
+        vm->def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE &&
+        virFileExists(vm->def->os.loader->nvram->path)) {
         if ((flags & VIR_DOMAIN_UNDEFINE_NVRAM)) {
-            if (unlink(vm->def->os.loader->nvram) < 0) {
+            if (unlink(vm->def->os.loader->nvram->path) < 0) {
                 virReportSystemError(errno,
                                      _("failed to remove nvram: %s"),
-                                     vm->def->os.loader->nvram);
+                                     vm->def->os.loader->nvram->path);
                 goto endjob;
             }
         } else if (!(flags & VIR_DOMAIN_UNDEFINE_KEEP_NVRAM)) {
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 37876b8..e4c05e2 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3994,25 +3994,41 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
     const char *master_nvram_path;
     ssize_t r;
 
-    if (!loader || !loader->nvram || virFileExists(loader->nvram))
+    /* return early if either loader is network-backed
+     * or NVRAM is already specified.
+     */
+    if (!loader || !loader->src || !loader->nvram ||
+        loader->type != VIR_DOMAIN_LOADER_TYPE_PFLASH ||
+        loader->src->type == VIR_STORAGE_TYPE_NETWORK ||
+        loader->nvram->type == VIR_STORAGE_TYPE_NETWORK)
+        return 0;
+
+    if (loader->nvram->type == VIR_STORAGE_TYPE_FILE &&
+        virFileExists(loader->nvram->path))
         return 0;
 
     master_nvram_path = loader->templt;
-    if (!loader->templt) {
+    /* Even if a template is not specified, we associate "known" EFI firmware
+     * to their NVRAM templates.
+     * Ofcourse this only applies to local firmware paths, as it is diffcult
+     * for libvirt to parse all network paths.
+     */
+    if (!loader->templt && loader->src->type == VIR_STORAGE_TYPE_FILE) {
         size_t i;
         for (i = 0; i < cfg->nfirmwares; i++) {
-            if (STREQ(cfg->firmwares[i]->name, loader->path)) {
+            if (STREQ(cfg->firmwares[i]->name, loader->src->path)) {
                 master_nvram_path = cfg->firmwares[i]->nvram;
                 break;
             }
         }
     }
 
-    if (!master_nvram_path) {
-        virReportError(VIR_ERR_OPERATION_FAILED,
-                       _("unable to find any master var store for "
-                         "loader: %s"), loader->path);
-        goto cleanup;
+    if (!master_nvram_path && loader->nvram) {
+        /* There is no template description, but an NVRAM spec
+         * has already been provided.
+         * Trust the client to have generated the right spec here
+         */
+        return 0;
     }
 
     if ((srcFD = virFileOpenAs(master_nvram_path, O_RDONLY,
@@ -4022,13 +4038,13 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
                              master_nvram_path);
         goto cleanup;
     }
-    if ((dstFD = virFileOpenAs(loader->nvram,
+    if ((dstFD = virFileOpenAs(loader->nvram->path,
                                O_WRONLY | O_CREAT | O_EXCL,
                                S_IRUSR | S_IWUSR,
                                cfg->user, cfg->group, 0)) < 0) {
         virReportSystemError(-dstFD,
                              _("Failed to create file '%s'"),
-                             loader->nvram);
+                             loader->nvram->path);
         goto cleanup;
     }
     created = true;
@@ -4046,7 +4062,7 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
         if (safewrite(dstFD, buf, r) < 0) {
             virReportSystemError(errno,
                                  _("Unable to write to file '%s'"),
-                                 loader->nvram);
+                                 loader->nvram->path);
             goto cleanup;
         }
     } while (r);
@@ -4060,7 +4076,7 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
     if (VIR_CLOSE(dstFD) < 0) {
         virReportSystemError(errno,
                              _("Unable to close file '%s'"),
-                             loader->nvram);
+                             loader->nvram->path);
         goto cleanup;
     }
 
@@ -4070,7 +4086,7 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
      * copy the file content. Roll back. */
     if (ret < 0) {
         if (created)
-            unlink(loader->nvram);
+            unlink(loader->nvram->path);
     }
 
     VIR_FORCE_CLOSE(srcFD);
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 8938e2d..3febea6 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -1604,7 +1604,8 @@ virSecurityDACRestoreAllLabel(virSecurityManagerPtr mgr,
     }
 
     if (def->os.loader && def->os.loader->nvram &&
-        virSecurityDACRestoreFileLabel(priv, def->os.loader->nvram) < 0)
+        def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE &&
+        virSecurityDACRestoreFileLabel(priv, def->os.loader->nvram->path) < 0)
         rc = -1;
 
     return rc;
@@ -1732,8 +1733,9 @@ virSecurityDACSetAllLabel(virSecurityManagerPtr mgr,
         return -1;
 
     if (def->os.loader && def->os.loader->nvram &&
+        def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE &&
         virSecurityDACSetOwnership(priv, NULL,
-                                   def->os.loader->nvram, user, group) < 0)
+                                   def->os.loader->nvram->path, user, group) < 0)
         return -1;
 
     if (def->os.kernel &&
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 5f74ef7..bcda939 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -2459,7 +2459,8 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManagerPtr mgr,
         rc = -1;
 
     if (def->os.loader && def->os.loader->nvram &&
-        virSecuritySELinuxRestoreFileLabel(mgr, def->os.loader->nvram) < 0)
+        def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE &&
+        virSecuritySELinuxRestoreFileLabel(mgr, def->os.loader->nvram->path) < 0)
         rc = -1;
 
     return rc;
@@ -2851,8 +2852,9 @@ virSecuritySELinuxSetAllLabel(virSecurityManagerPtr mgr,
     /* This is different than kernel or initrd. The nvram store
      * is really a disk, qemu can read and write to it. */
     if (def->os.loader && def->os.loader->nvram &&
+        def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE &&
         secdef && secdef->imagelabel &&
-        virSecuritySELinuxSetFilecon(mgr, def->os.loader->nvram,
+        virSecuritySELinuxSetFilecon(mgr, def->os.loader->nvram->path,
                                      secdef->imagelabel) < 0)
         return -1;
 
-- 
1.8.1.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 06/12] Fix the domain def inference logic to correctly account for network-backed pflash devices.
Posted by Prerna Saxena, 1 week ago
Signed-off-by: Prerna Saxena <saxenap.ltc@gmail.com>
---
 src/qemu/qemu_parse_command.c | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c
index 351425f..9b1a86e 100644
--- a/src/qemu/qemu_parse_command.c
+++ b/src/qemu/qemu_parse_command.c
@@ -650,6 +650,7 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt,
     int idx = -1;
     int busid = -1;
     int unitid = -1;
+    bool is_firmware = false;
 
     if (qemuParseKeywords(val,
                           &keywords,
@@ -772,6 +773,9 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt,
                 def->bus = VIR_DOMAIN_DISK_BUS_VIRTIO;
             } else if (STREQ(values[i], "xen")) {
                 def->bus = VIR_DOMAIN_DISK_BUS_XEN;
+            } else if (STREQ(values[i], "pflash")) {
+                def->bus = VIR_DOMAIN_DISK_BUS_LAST;
+                is_firmware = true;
             } else if (STREQ(values[i], "sd")) {
                 def->bus = VIR_DOMAIN_DISK_BUS_SD;
             }
@@ -943,8 +947,25 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt,
         ignore_value(VIR_STRDUP(def->dst, "hda"));
     }
 
-    if (!def->dst)
-        goto error;
+    if (!def->dst) {
+        if (is_firmware && def->bus == VIR_DOMAIN_DISK_BUS_LAST) {
+            if (!dom->os.loader && (VIR_ALLOC(dom->os.loader) < 0))
+                goto error;
+            if (def->src->readonly) {
+                /* Loader spec */
+                dom->os.loader->src = def->src;
+                dom->os.loader->type = VIR_DOMAIN_LOADER_TYPE_PFLASH;
+            } else {
+                /* NVRAM Spec */
+                if (!dom->os.loader->nvram && (VIR_ALLOC(dom->os.loader->nvram) < 0))
+                    goto error;
+                dom->os.loader->nvram = def->src;
+            }
+        } else {
+            goto error;
+        }
+    }
+
     if (STREQ(def->dst, "xvda"))
         def->dst[3] = 'a' + idx;
     else
@@ -2215,8 +2236,11 @@ qemuParseCommandLine(virCapsPtr caps,
         } else if (STREQ(arg, "-bios")) {
             WANT_VALUE();
             if (VIR_ALLOC(def->os.loader) < 0 ||
-                VIR_STRDUP(def->os.loader->path, val) < 0)
+                VIR_ALLOC(def->os.loader->src) < 0 ||
+                VIR_STRDUP(def->os.loader->src->path, val) < 0)
                 goto error;
+            def->os.loader->src->type = VIR_STORAGE_TYPE_FILE;
+            def->os.loader->type = VIR_DOMAIN_LOADER_TYPE_ROM;
         } else if (STREQ(arg, "-initrd")) {
             WANT_VALUE();
             if (VIR_STRDUP(def->os.initrd, val) < 0)
-- 
1.8.1.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 07/12] Bhyve: Fix command line generation to correctly pick up local loader path.
Posted by Prerna Saxena, 1 week ago
Signed-off-by: Prerna Saxena <saxenap.ltc@gmail.com>
---
 src/bhyve/bhyve_command.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
index 9413ae5..2b67014 100644
--- a/src/bhyve/bhyve_command.c
+++ b/src/bhyve/bhyve_command.c
@@ -518,10 +518,12 @@ virBhyveProcessBuildBhyveCmd(virConnectPtr conn,
     virCommandAddArgList(cmd, "-s", "0:0,hostbridge", NULL);
 
     if (def->os.bootloader == NULL &&
-        def->os.loader) {
+        def->os.loader &&
+        def->os.loader->src &&
+        def->os.loader->src->type == VIR_STORAGE_TYPE_FILE) {
         if ((bhyveDriverGetCaps(conn) & BHYVE_CAP_LPC_BOOTROM)) {
             virCommandAddArg(cmd, "-l");
-            virCommandAddArgFormat(cmd, "bootrom,%s", def->os.loader->path);
+            virCommandAddArgFormat(cmd, "bootrom,%s", def->os.loader->src->path);
             add_lpc = true;
         } else {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-- 
1.8.1.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 08/12] virt-aa-helper: Adjust references to loader & nvram elements to correctly parse the virStorageSource types.
Posted by Prerna Saxena, 1 week ago
Signed-off-by: Prerna Saxena <saxenap.ltc@gmail.com>
---
 src/security/virt-aa-helper.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index d0f9876..8217d67 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -1063,12 +1063,18 @@ get_files(vahControl * ctl)
         if (vah_add_file(&buf, ctl->def->os.slic_table, "r") != 0)
             goto cleanup;
 
-    if (ctl->def->os.loader && ctl->def->os.loader->path)
-        if (vah_add_file(&buf, ctl->def->os.loader->path, "rk") != 0)
+    if (ctl->def->os.loader &&
+        ctl->def->os.loader->src &&
+        ctl->def->os.loader->src->type == VIR_STORAGE_TYPE_FILE &&
+        ctl->def->os.loader->src->path)
+        if (vah_add_file(&buf, ctl->def->os.loader->src->path, "rk") != 0)
             goto cleanup;
 
-    if (ctl->def->os.loader && ctl->def->os.loader->nvram)
-        if (vah_add_file(&buf, ctl->def->os.loader->nvram, "rwk") != 0)
+    if (ctl->def->os.loader &&
+        ctl->def->os.loader->nvram &&
+        ctl->def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE &&
+        ctl->def->os.loader->nvram->path)
+        if (vah_add_file(&buf, ctl->def->os.loader->nvram->path, "rwk") != 0)
             goto cleanup;
 
     for (i = 0; i < ctl->def->ngraphics; i++) {
-- 
1.8.1.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 09/12] Vbox: Adjust references to 'loader' and 'nvram' elements given that these are now represented by virStorageSourcePtr.
Posted by Prerna Saxena, 1 week ago
Signed-off-by: Prerna Saxena <saxenap.ltc@gmail.com>
---
 src/vbox/vbox_common.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 72a24a3..60451a3 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -998,11 +998,16 @@ vboxSetBootDeviceOrder(virDomainDefPtr def, vboxDriverPtr data,
     VIR_DEBUG("def->os.initrd           %s", def->os.initrd);
     VIR_DEBUG("def->os.cmdline          %s", def->os.cmdline);
     VIR_DEBUG("def->os.root             %s", def->os.root);
-    if (def->os.loader) {
-        VIR_DEBUG("def->os.loader->path     %s", def->os.loader->path);
+    if (def->os.loader &&
+        def->os.loader->src &&
+        def->os.loader->src->type == VIR_STORAGE_TYPE_FILE) {
+
+        VIR_DEBUG("def->os.loader->src->path     %s", def->os.loader->src->path);
         VIR_DEBUG("def->os.loader->readonly %d", def->os.loader->readonly);
         VIR_DEBUG("def->os.loader->type     %d", def->os.loader->type);
-        VIR_DEBUG("def->os.loader->nvram    %s", def->os.loader->nvram);
+        if (def->os.loader->nvram &&
+            def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE)
+            VIR_DEBUG("def->os.loader->nvram->path    %s", def->os.loader->nvram->path);
     }
     VIR_DEBUG("def->os.bootloader       %s", def->os.bootloader);
     VIR_DEBUG("def->os.bootloaderArgs   %s", def->os.bootloaderArgs);
-- 
1.8.1.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 10/12] Xen: Adjust all references to loader & nvram elements given that they are now backed by virStorageSourcePtr
Posted by Prerna Saxena, 1 week ago
Signed-off-by: Prerna Saxena <saxenap.ltc@gmail.com>
---
 src/xenapi/xenapi_driver.c |  4 +++-
 src/xenconfig/xen_sxpr.c   | 19 +++++++++++--------
 src/xenconfig/xen_xm.c     |  9 ++++++---
 3 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c
index 42b305d..4070660 100644
--- a/src/xenapi/xenapi_driver.c
+++ b/src/xenapi/xenapi_driver.c
@@ -1444,10 +1444,12 @@ xenapiDomainGetXMLDesc(virDomainPtr dom, unsigned int flags)
         char *value = NULL;
         defPtr->os.type = VIR_DOMAIN_OSTYPE_XEN;
         if (VIR_ALLOC(defPtr->os.loader) < 0 ||
-            VIR_STRDUP(defPtr->os.loader->path, "pygrub") < 0) {
+            VIR_ALLOC(defPtr->os.loader->src) < 0 ||
+            VIR_STRDUP(defPtr->os.loader->src->path, "pygrub") < 0) {
             VIR_FREE(boot_policy);
             goto error;
         }
+        defPtr->os.loader->src->type = VIR_STORAGE_TYPE_FILE;
         xen_vm_get_pv_kernel(session, &value, vm);
         if (STRNEQ(value, "")) {
             if (VIR_STRDUP(defPtr->os.kernel, value) < 0) {
diff --git a/src/xenconfig/xen_sxpr.c b/src/xenconfig/xen_sxpr.c
index e868c05..fd3165c 100644
--- a/src/xenconfig/xen_sxpr.c
+++ b/src/xenconfig/xen_sxpr.c
@@ -87,15 +87,17 @@ xenParseSxprOS(const struct sexpr *node,
                int hvm)
 {
     if (hvm) {
-        if (VIR_ALLOC(def->os.loader) < 0)
+        if ((VIR_ALLOC(def->os.loader) < 0) ||
+            (VIR_ALLOC(def->os.loader->src) < 0))
             goto error;
-        if (sexpr_node_copy(node, "domain/image/hvm/loader", &def->os.loader->path) < 0)
+        def->os.loader->src->type = VIR_STORAGE_TYPE_FILE;
+        if (sexpr_node_copy(node, "domain/image/hvm/loader", &def->os.loader->src->path) < 0)
             goto error;
-        if (def->os.loader->path == NULL) {
-            if (sexpr_node_copy(node, "domain/image/hvm/kernel", &def->os.loader->path) < 0)
+        if (def->os.loader->src->path == NULL) {
+            if (sexpr_node_copy(node, "domain/image/hvm/kernel", &def->os.loader->src->path) < 0)
                 goto error;
 
-            if (def->os.loader->path == NULL) {
+            if (def->os.loader->src->path == NULL) {
                 virReportError(VIR_ERR_INTERNAL_ERROR,
                                "%s", _("domain information incomplete, missing HVM loader"));
                 return -1;
@@ -124,7 +126,8 @@ xenParseSxprOS(const struct sexpr *node,
     /* If HVM kenrel == loader, then old xend, so kill off kernel */
     if (hvm &&
         def->os.kernel &&
-        STREQ(def->os.kernel, def->os.loader->path)) {
+        def->os.loader->src &&
+        STREQ(def->os.kernel, def->os.loader->src->path)) {
         VIR_FREE(def->os.kernel);
     }
     /* Drop kernel argument that has no value */
@@ -2259,9 +2262,9 @@ xenFormatSxpr(virConnectPtr conn, virDomainDefPtr def)
         if (hvm) {
             char bootorder[VIR_DOMAIN_BOOT_LAST+1];
             if (def->os.kernel)
-                virBufferEscapeSexpr(&buf, "(loader '%s')", def->os.loader->path);
+                virBufferEscapeSexpr(&buf, "(loader '%s')", def->os.loader->src->path);
             else
-                virBufferEscapeSexpr(&buf, "(kernel '%s')", def->os.loader->path);
+                virBufferEscapeSexpr(&buf, "(kernel '%s')", def->os.loader->src->path);
 
             virBufferAsprintf(&buf, "(vcpus %u)", virDomainDefGetVcpusMax(def));
             if (virDomainDefHasVcpusOffline(def))
diff --git a/src/xenconfig/xen_xm.c b/src/xenconfig/xen_xm.c
index 8ef68bb..c6a1f03 100644
--- a/src/xenconfig/xen_xm.c
+++ b/src/xenconfig/xen_xm.c
@@ -47,8 +47,10 @@ xenParseXMOS(virConfPtr conf, virDomainDefPtr def)
         const char *boot;
 
         if (VIR_ALLOC(def->os.loader) < 0 ||
-            xenConfigCopyString(conf, "kernel", &def->os.loader->path) < 0)
+            VIR_ALLOC(def->os.loader->src) < 0 ||
+            xenConfigCopyString(conf, "kernel", &def->os.loader->src->path) < 0)
             return -1;
+        def->os.loader->src->type = VIR_STORAGE_TYPE_FILE;
 
         if (xenConfigGetString(conf, "boot", &boot, "c") < 0)
             return -1;
@@ -481,9 +483,10 @@ xenFormatXMOS(virConfPtr conf, virDomainDefPtr def)
         if (xenConfigSetString(conf, "builder", "hvm") < 0)
             return -1;
 
-        if (def->os.loader && def->os.loader->path &&
-            xenConfigSetString(conf, "kernel", def->os.loader->path) < 0)
+        if (def->os.loader && def->os.loader->src &&
+            xenConfigSetString(conf, "kernel", def->os.loader->src->path) < 0)
             return -1;
+        def->os.loader->src->type = VIR_STORAGE_TYPE_FILE;
 
         for (i = 0; i < def->os.nBootDevs; i++) {
             switch (def->os.bootDevs[i]) {
-- 
1.8.1.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 11/12] Test: Add a test snippet to evaluate command line generation for loader/nvram specified via virStorageSource
Posted by Prerna Saxena, 1 week ago
Signed-off-by: Prerna Saxena <saxenap.ltc@gmail.com>
---
 tests/qemuxml2argvdata/bios-nvram-network.args | 31 +++++++++++++++++++
 tests/qemuxml2argvdata/bios-nvram-network.xml  | 42 ++++++++++++++++++++++++++
 tests/qemuxml2argvtest.c                       |  1 +
 3 files changed, 74 insertions(+)
 create mode 100644 tests/qemuxml2argvdata/bios-nvram-network.args
 create mode 100644 tests/qemuxml2argvdata/bios-nvram-network.xml

diff --git a/tests/qemuxml2argvdata/bios-nvram-network.args b/tests/qemuxml2argvdata/bios-nvram-network.args
new file mode 100644
index 0000000..3fc68ef
--- /dev/null
+++ b/tests/qemuxml2argvdata/bios-nvram-network.args
@@ -0,0 +1,31 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-x86_64 \
+-name test-bios \
+-S \
+-machine pc,accel=tcg,usb=off,dump-guest-core=off \
+-drive file=/usr/share/OVMF/OVMF_CODE.fd,if=pflash,format=raw,unit=0,\
+readonly=on \
+-drive file=iscsi://example.org:6000/iqn.1992-01.com.example/0,\
+if=pflash,format=raw,unit=1 \
+-m 1024 \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid 362d1fc1-df7d-193e-5c18-49a71bd1da66 \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-test-bios/monitor.sock,\
+server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-boot order=c,menu=on \
+-usb \
+-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
+-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
+-device usb-tablet,id=input0,bus=usb.0,port=1 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/bios-nvram-network.xml b/tests/qemuxml2argvdata/bios-nvram-network.xml
new file mode 100644
index 0000000..bad114c
--- /dev/null
+++ b/tests/qemuxml2argvdata/bios-nvram-network.xml
@@ -0,0 +1,42 @@
+<domain type='qemu'>
+  <name>test-bios</name>
+  <uuid>362d1fc1-df7d-193e-5c18-49a71bd1da66</uuid>
+  <memory unit='KiB'>1048576</memory>
+  <currentMemory unit='KiB'>1048576</currentMemory>
+  <vcpu placement='static'>1</vcpu>
+  <os>
+    <type arch='x86_64' machine='pc'>hvm</type>
+    <loader readonly='yes' type='pflash' backing='file'>
+      <source file='/usr/share/OVMF/OVMF_CODE.fd'/>
+    </loader>
+    <nvram backing='network'>
+      <source protocol='iscsi' name='iqn.1992-01.com.example'>
+        <host name='example.org' port='6000'/>
+      </source>
+    </nvram>
+    <boot dev='hd'/>
+    <bootmenu enable='yes'/>
+  </os>
+  <features>
+    <acpi/>
+  </features>
+  <clock offset='utc'/>
+  <on_poweroff>destroy</on_poweroff>
+  <on_reboot>restart</on_reboot>
+  <on_crash>restart</on_crash>
+  <devices>
+    <emulator>/usr/bin/qemu-system-x86_64</emulator>
+    <disk type='block' device='disk'>
+      <source dev='/dev/HostVG/QEMUGuest1'/>
+      <target dev='hda' bus='ide'/>
+      <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+    </disk>
+    <controller type='usb' index='0'/>
+    <controller type='ide' index='0'/>
+    <controller type='pci' index='0' model='pci-root'/>
+    <input type='tablet' bus='usb'/>
+    <input type='mouse' bus='ps2'/>
+    <input type='keyboard' bus='ps2'/>
+    <memballoon model='virtio'/>
+  </devices>
+</domain>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index ddf567b..7338dba 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -853,6 +853,7 @@ mymain(void)
             QEMU_CAPS_DEVICE_ISA_SERIAL,
             QEMU_CAPS_SGA);
     DO_TEST("bios-nvram", NONE);
+    DO_TEST("bios-nvram-network", NONE);
     DO_TEST("bios-nvram-secure",
             QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
             QEMU_CAPS_DEVICE_PCI_BRIDGE,
-- 
1.8.1.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 12/12] Documentation: Add a blurb for the newly added XML snippets for loader and nvram.
Posted by Prerna Saxena, 1 week ago
Signed-off-by: Prerna Saxena <saxenap.ltc@gmail.com>
---
 docs/formatdomain.html.in | 36 +++++++++++++++++++++++++++++++++---
 1 file changed, 33 insertions(+), 3 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index caeb14e..b8cb7ac 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -112,6 +112,20 @@
 &lt;/os&gt;
 ...</pre>
 
+    <p>Where loader/NVRAM can also be described as:</p>
+
+<pre>
+...
+  &lt;loader readonly='yes' secure='no' type='rom' backing='file'&gt;
+    &lt;source file='/usr/share/OVMF/OVMF_CODE.fd'/&gt;
+  &lt;/loader&gt;
+  &lt;nvram backing='network' template='/usr/share/OVMF/OVMF_VARS.fd'&gt;
+    &lt;source protocol='iscsi' name='iqn.2013-07.com.example:iscsi-nopool/2'&gt;
+      &lt;host name='example.com' port='3260'/&gt;
+    &lt;/source&gt;
+  &lt;/nvram&gt;
+...</pre>
+
     <dl>
       <dt><code>type</code></dt>
       <dd>The content of the <code>type</code> element specifies the
@@ -143,7 +157,15 @@
         <code>pflash</code>. Moreover, some firmwares may
         implement the Secure boot feature. Attribute
         <code>secure</code> can be used then to control it.
-        <span class="since">Since 2.1.0</span></dd>
+        <span class="since">Since 2.1.0</span><span class="since">Since 4.5.0</span>
+        Loader element can also be specified as a remote disk. The attribute
+        <code>backing</code> (accepted values are
+        <code>file</code> and <code>network</code>)can be used to represent
+        local absolute paths as well as network-backed disks. The details of
+        backing file may be specified as <a href="#elementsDiskSource">storage source</a>
+        Allowed backing type <code>file</code> replaces the old
+        specification and extends to all hypervisors that supported it, while
+        type <code>network</code> is only supported by QEMU.</dd>
       <dt><code>nvram</code></dt>
       <dd>Some UEFI firmwares may want to use a non-volatile memory to store
         some variables. In the host, this is represented as a file and the
@@ -154,7 +176,15 @@
         from the config file. Note, that for transient domains if the NVRAM file
         has been created by libvirt it is left behind and it is management
         application's responsibility to save and remove file (if needed to be
-        persistent). <span class="since">Since 1.2.8</span></dd>
+        persistent). <span class="since">Since 1.2.8</span>
+        <span class="since">Since 4.5.0</span>, attribute
+        <code>backing</code>(accepted values <code>file</code>
+        and <code>network</code>)can be used to specify a
+        <a href='#elementsDiskSource'>storage source</a>
+        of type file or network. The value <code>file</code>describes
+        absolute NVRAM paths and extends the present specification
+        for all hypervisors that support this, while
+        <code>network</code> applies only to QEMU.</dd>
       <dt><code>boot</code></dt>
       <dd>The <code>dev</code> attribute takes one of the values "fd", "hd",
         "cdrom" or "network" and is used to specify the next boot device
@@ -2707,7 +2737,7 @@
             </dd>
         </dl>
       </dd>
-      <dt><code>source</code></dt>
+      <h5><a id="elementsDiskSource">Disk source</a></h5>
       <dd>Representation of the disk <code>source</code> depends on the
       disk <code>type</code> attribute value as follows:
           <dl>
-- 
1.8.1.2

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