Re: [libvirt] [PATCH] Qemu driver: Support network-backed pflash disks.

[John Ferlan]

On 05/02/2018 01:24 PM, Prerna wrote:
> 
> 
> On Sat, Apr 28, 2018 at 1:11 AM, John Ferlan  > wrote:
> 
> 
> 

[...]

>  
> 
> This will need a v2 anyway because the patch has too much going on and
> needs to be split up more.
> 
> 
> Will do. I should have properly mentioned this was an RFC rather than a
> ready-to-merge patch, and that this currently excluded test and
> documentation fixes.
>  

Even with an RFC - it's really more pleasant to break things up a bit.
Makes it easier to follow flow rather than one large patch where
multiple things are happening and you're left trying to figure that all
out.  But I also understand that sometimes when posting as RFC that
things languish longer than posting as v1, so it's a gamble as to which
to use...

> 
> 
> You need to break out the domain_conf, docs, etc. changes into one
> patch. Much of what you put into the cover that describes the XML
> should 
> 
> 
> Got it.
>  
> 
> go into this patch's commit message. There should be xml2xml test
> changes as well as adjustments to formatdomain.html.in
>  to describe the
> new options. The part of the cover that says automatically reformatting
> to use the storage source cannot happen. There's precedence for that 
> 
> when the  and  moved *into* the storage source we
> still have to accommodate for them outside. Internally, it can be placed
> as expected, but when formatting, we have to format as we read. After
> 
> 
> Ok. I had explicitly asked around on IRC if it was okay "breaking" the
> existing XML  semantics. Peter had mentioned it was okay to have the XML
> read as its old semantic. The formatter could later "translate" the old
> -style absolute filepaths into the "new-style" source-description that
> is introduced.
> I had kept that in mind while implementing this patch. If that is not to
> be done, I will need to redo parts of this patch.
>  

I see you pinged on IRC today - I had this marked as get back to because
it appears there's questions. Juggling lots of different series and your
response only came on Wed afternoon for me. It's now Friday morning.

In any case, I think that contradicts what was required of me to be done
when moving  and  into  from . See
commits 37537a7c and 8002d3c which handle formatting as the XML was read.

Now if someone *changes* the read XML to use , then
that's a different story. But if you find:

/usr/lib/xen/boot/hvmloader

then you should format that.

If you find


  


then you format that.

Hopefully the two examples provided give you an idea of the "accepted
mechanism" used in a previous change of XML format.

> 
> that, perhaps add the security changes in another, the cgroup in
> another, and finally the qemu adjustments in the last.  Not even sure if
> you need a capability as well.
> 
> 
> Why do you think we'd need a capability for this?  I'd be keen to
> understand why changes to XML spec  is not enough.
>  

Depends on the command line generated I guess - cannot recall right now
if that was clear while I was looking at the existing changes. In the
long run, you have to decide whether the arguments provided to QEMU have
existed since QEMU 1.5. If they have, then no capability, but if there's
some argument provided on the QEMU command line that was introduced in
(for example) 2.9 in order to allow usage of a networked path, then
you'd need a capability.

> 
> Finally this doesn't even compile for me.  You removed @path from
> _virDomainLoaderDef but neglected to adjust all consumers. I suggest
> using cscope and egrep'ing on "os.*loader->path" as well as ->nvram
> since you changed it's type.
> 
> 
> I know why. I had run and tested this to work, but my build
> configuration included the qemu driver and excluded every other driver.
> Given that this element extends to other drivers, I can understand the
> build issues. Again, should have mentioned this was an RFC.
>  
> 
> 
> I assume you've considered that network storage types need to deal with
> authentication and encryption passphrases, right?  What about using a
> srcpool storage source?
> 
> 
> Erm, no. This patch does not include support for
> encryption/authenticaton. I will need to add those.
>  

At least a storage source provides the capability to storage, parse,
format that data...

> 
> I'll briefly scan the rest.
> 

[...]

> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index 35666c1..c80f9d9 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -2883,8 +2883,8 @@ virDomainLoaderDefFree(virDomainLoaderDefPtr
> loader)
> >      if (!loader)
> >          return;
> > 
> > -    VIR_FREE(loader->path);
> > -    VIR_FREE(loader->nvram);
> > +    virStorageSourceFree(loader->loader_src);
> 
> loader_src is redundant to loader isn't 

Re: [libvirt] [PATCH] Qemu driver: Support network-backed pflash disks.

[Prerna]

On Sat, Apr 28, 2018 at 1:11 AM, John Ferlan  wrote:

>
>
> On 04/20/2018 04:59 AM, Prerna Saxena wrote:
> > So far libvirt domain XML 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.
> >
> > Signed-off-by: Prerna Saxena 
> > ---
> >  docs/schemas/domaincommon.rng   | 108 +++
> >  src/conf/domain_conf.c  | 188 ++
> ++
> >  src/conf/domain_conf.h  |   7 +-
> >  src/qemu/qemu_cgroup.c  |  13 ++-
> >  src/qemu/qemu_command.c |  21 +++--
> >  src/qemu/qemu_domain.c  |  16 ++--
> >  src/qemu/qemu_driver.c  |   7 +-
> >  src/qemu/qemu_parse_command.c   |  30 ++-
> >  src/qemu/qemu_process.c |  33 ---
> >  src/security/security_dac.c |   6 +-
> >  src/security/security_selinux.c |   6 +-
> >  11 files changed, 361 insertions(+), 74 deletions(-)
> >
>
> I know on IRC you asked Peter or Michal to review (and they're CC'd
> here), but before they get a chance next week some time - I'll give you
>

Thank you for taking a look.


> a quick look. You do understand Peter, Michal, myself, and other Red Hat
> libvirt developers follow libvir-list anyway - so CC'ing any of us
> doesn't do much since we filter into a mail folder anyway...
>
>
I understand. Peter & Michal had participated in the original IRC
discussion around the need for extending loader as a network backed device,
and I had cc'ed them for context.


> This will need a v2 anyway because the patch has too much going on and
> needs to be split up more.
>

Will do. I should have properly mentioned this was an RFC rather than a
ready-to-merge patch, and that this currently excluded test and
documentation fixes.


>
> You need to break out the domain_conf, docs, etc. changes into one
> patch. Much of what you put into the cover that describes the XML should


Got it.


> go into this patch's commit message. There should be xml2xml test
> changes as well as adjustments to formatdomain.html.in to describe the
> new options. The part of the cover that says automatically reformatting
> to use the storage source cannot happen. There's precedence for that

when the  and  moved *into* the storage source we
> still have to accommodate for them outside. Internally, it can be placed
> as expected, but when formatting, we have to format as we read. After
>

Ok. I had explicitly asked around on IRC if it was okay "breaking" the
existing XML  semantics. Peter had mentioned it was okay to have the XML
read as its old semantic. The formatter could later "translate" the old
-style absolute filepaths into the "new-style" source-description that is
introduced.
I had kept that in mind while implementing this patch. If that is not to be
done, I will need to redo parts of this patch.


> that, perhaps add the security changes in another, the cgroup in
> another, and finally the qemu adjustments in the last.  Not even sure if
> you need a capability as well.
>
>
Why do you think we'd need a capability for this?  I'd be keen to
understand why changes to XML spec  is not enough.


> Finally this doesn't even compile for me.  You removed @path from
> _virDomainLoaderDef but neglected to adjust all consumers. I suggest
> using cscope and egrep'ing on "os.*loader->path" as well as ->nvram
> since you changed it's type.
>

I know why. I had run and tested this to work, but my build configuration
included the qemu driver and excluded every other driver. Given that this
element extends to other drivers, I can understand the build issues. Again,
should have mentioned this was an RFC.


>
> I assume you've considered that network storage types need to deal with
> authentication and encryption passphrases, right?  What about using a
> srcpool storage source?
>
>
Erm, no. This patch does not include support for encryption/authenticaton.
I will need to add those.


> I'll briefly scan the rest.
>
> > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.
> rng
> > index 4cab55f..32db395 100644
> > --- a/docs/schemas/domaincommon.rng
> > +++ b/docs/schemas/domaincommon.rng
> > @@ -276,7 +276,42 @@
> >  
> >
> >  
> > -
> > +
> > +  
> > +
> > +  file
> > +  network
> > +
> > +  
> > +
> > +
> > +  
> > + 
> > +  
> > + 
> > + 
> > +  
> > + 
> > + 
> > +  
> > + 
> > + 
> > +  
> > + 
> > + 
> > + 

Re: [libvirt] [PATCH] Qemu driver: Support network-backed pflash disks.

[John Ferlan]

On 04/20/2018 04:59 AM, Prerna Saxena wrote:
> So far libvirt domain XML 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.
> 
> Signed-off-by: Prerna Saxena 
> ---
>  docs/schemas/domaincommon.rng   | 108 +++
>  src/conf/domain_conf.c  | 188 
> 
>  src/conf/domain_conf.h  |   7 +-
>  src/qemu/qemu_cgroup.c  |  13 ++-
>  src/qemu/qemu_command.c |  21 +++--
>  src/qemu/qemu_domain.c  |  16 ++--
>  src/qemu/qemu_driver.c  |   7 +-
>  src/qemu/qemu_parse_command.c   |  30 ++-
>  src/qemu/qemu_process.c |  33 ---
>  src/security/security_dac.c |   6 +-
>  src/security/security_selinux.c |   6 +-
>  11 files changed, 361 insertions(+), 74 deletions(-)
> 

I know on IRC you asked Peter or Michal to review (and they're CC'd
here), but before they get a chance next week some time - I'll give you
a quick look. You do understand Peter, Michal, myself, and other Red Hat
libvirt developers follow libvir-list anyway - so CC'ing any of us
doesn't do much since we filter into a mail folder anyway...

This will need a v2 anyway because the patch has too much going on and
needs to be split up more.

You need to break out the domain_conf, docs, etc. changes into one
patch. Much of what you put into the cover that describes the XML should
go into this patch's commit message. There should be xml2xml test
changes as well as adjustments to formatdomain.html.in to describe the
new options. The part of the cover that says automatically reformatting
to use the storage source cannot happen. There's precedence for that
when the  and  moved *into* the storage source we
still have to accommodate for them outside. Internally, it can be placed
as expected, but when formatting, we have to format as we read. After
that, perhaps add the security changes in another, the cgroup in
another, and finally the qemu adjustments in the last.  Not even sure if
you need a capability as well.

Finally this doesn't even compile for me.  You removed @path from
_virDomainLoaderDef but neglected to adjust all consumers. I suggest
using cscope and egrep'ing on "os.*loader->path" as well as ->nvram
since you changed it's type.

I assume you've considered that network storage types need to deal with
authentication and encryption passphrases, right?  What about using a
srcpool storage source?

I'll briefly scan the rest.

> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 4cab55f..32db395 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -276,7 +276,42 @@
>  
>
>  
> -
> +
> +  
> +
> +  file
> +  network
> +
> +  
> +
> +
> +  
> + 
> +  
> + 
> + 
> +  
> + 
> + 
> +  
> + 
> + 
> +  
> + 
> + 
> +  
> + 
> + 
> +  
> + 
> + 
> +  
> + 
> + 
> +  
> + 
> +  
> +
>
>  
>  
> @@ -287,7 +322,40 @@
>
>  
>  
> -  
> +  
> +
> +  file
> +  network
> +
> +  
> +
> +
> +  
> + 
> +  
> + 
> + 
> +  
> + 
> + 
> +  
> + 
> + 
> +  
> + 
> + 
> +  
> + 
> + 
> +  
> + 
> + 
> +  
> + 
> + 
> +  
> + 
> +  
>  
>
>  
> @@ -1494,25 +1562,29 @@
>
>  
>  
> -  
> -
> -  
> -
> -  
> -
> -
> -  
> -
> -
> -  
> -
> -
> -  
> -
> -  
> +  
>  
>
>  
> +  
> +
> +  
> +
> +  
> +
> +  
> +  
> +
> +  
> +  
> +
> + 

[libvirt] [PATCH] Qemu driver: Support network-backed pflash disks.

[Prerna Saxena]

So far libvirt domain XML 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.

Signed-off-by: Prerna Saxena 
---
 docs/schemas/domaincommon.rng   | 108 +++
 src/conf/domain_conf.c  | 188 
 src/conf/domain_conf.h  |   7 +-
 src/qemu/qemu_cgroup.c  |  13 ++-
 src/qemu/qemu_command.c |  21 +++--
 src/qemu/qemu_domain.c  |  16 ++--
 src/qemu/qemu_driver.c  |   7 +-
 src/qemu/qemu_parse_command.c   |  30 ++-
 src/qemu/qemu_process.c |  33 ---
 src/security/security_dac.c |   6 +-
 src/security/security_selinux.c |   6 +-
 11 files changed, 361 insertions(+), 74 deletions(-)

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 4cab55f..32db395 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -276,7 +276,42 @@
 
   
 
-
+
+  
+
+  file
+  network
+
+  
+
+
+  
+ 
+  
+ 
+ 
+  
+ 
+ 
+  
+ 
+ 
+  
+ 
+ 
+  
+ 
+ 
+  
+ 
+ 
+  
+ 
+ 
+  
+ 
+  
+
   
 
 
@@ -287,7 +322,40 @@
   
 
 
-  
+  
+
+  file
+  network
+
+  
+
+
+  
+ 
+  
+ 
+ 
+  
+ 
+ 
+  
+ 
+ 
+  
+ 
+ 
+  
+ 
+ 
+  
+ 
+ 
+  
+ 
+ 
+  
+ 
+  
 
   
 
@@ -1494,25 +1562,29 @@
   
 
 
-  
-
-  
-
-  
-
-
-  
-
-
-  
-
-
-  
-
-  
+  
 
   
 
+  
+
+  
+
+  
+
+  
+  
+
+  
+  
+
+  
+  
+
+  
+
+  
+
   
 
   block
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 35666c1..c80f9d9 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2883,8 +2883,8 @@ virDomainLoaderDefFree(virDomainLoaderDefPtr loader)
 if (!loader)
 return;
 
-VIR_FREE(loader->path);
-VIR_FREE(loader->nvram);
+virStorageSourceFree(loader->loader_src);
+virStorageSourceFree(loader->nvram);
 VIR_FREE(loader->templt);
 VIR_FREE(loader);
 }
@@ -17961,17 +17961,59 @@ 
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->loader_src)) {
+goto cleanup;
+}
+loader->loader_src->type = VIR_STORAGE_TYPE_LAST;
 
 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->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")) {
+if (virDomainStorageSourceParse(cur, cur_ctxt, loader->loader_src, 
0) < 0) {
+virReportError(VIR_ERR_XML_DETAIL,
+   

[libvirt] [PATCH] Qemu driver: Support network-backed pflash disks.

[Prerna Saxena]

This implements support for firmware loader & NVRAM disks over network-backed 
disks.
As discussed in 
https://www.redhat.com/archives/libvir-list/2018-March/msg01721.html,
the patch embeds the  spec for disks in  and  elements 
as well.

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:


  

  
  


The patche automatically re-formats any older-stype declaration into this new 
style.
Templates can be used to create a new NVRAM only if the nvram backing = 'file'.

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