Re: [Xen-devel] [OSSTEST PATCH 10/16] ts-debian-fixup: remove extra= before appending our own

[Wei Liu]

On Fri, Oct 20, 2017 at 01:52:50PM +0100, Ian Jackson wrote:
> Wei Liu writes ("Re: [OSSTEST PATCH 10/16] ts-debian-fixup: remove extra= 
> before appending our own"):
> > On Fri, Oct 20, 2017 at 12:03:24PM +0100, Ian Jackson wrote:
> > > Wei Liu writes ("[OSSTEST PATCH 10/16] ts-debian-fixup: remove extra= 
> > > before appending our own"):
> > > > The original extra= was not removed, so there were two extra= in the
> > > > resulting config file.
> > > 
> > > What is the original extra= ?  Why should we not combine them ?
> > 
> > The original extra= is generated by xen-create-image. It has the content
> > "elevator=noop". It doesn't seem too useful to me. But I'm fine with
> > combination them.
> 
> I think they should be combined.  And that "elevator=noop" doesn't
> sound unreasonable.
> 
> > > > It wasn't a problem for xl because the second extra= took precedence.
> > > > However libvirt tests would only pick up the first extra= --  they
> > > > worked by chance.
> > > 
> > > That's odd.  Is this to do with the xl -> libxl config converter ?
> > > It seems to me that that converter should interpret xl config files
> > > the same way xl does.  (Also xm did the same: last setting wins.)
> > 
> > Yes, the converter only picks up the first.
> 
> This is a bug, then.  Where should we file it ?
> 

Not sure -- maybe email Jim and CC libvirt list?

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [OSSTEST PATCH 10/16] ts-debian-fixup: remove extra= before appending our own

[Ian Jackson]

Wei Liu writes ("Re: [OSSTEST PATCH 10/16] ts-debian-fixup: remove extra= 
before appending our own"):
> On Fri, Oct 20, 2017 at 12:03:24PM +0100, Ian Jackson wrote:
> > Wei Liu writes ("[OSSTEST PATCH 10/16] ts-debian-fixup: remove extra= 
> > before appending our own"):
> > > The original extra= was not removed, so there were two extra= in the
> > > resulting config file.
> > 
> > What is the original extra= ?  Why should we not combine them ?
> 
> The original extra= is generated by xen-create-image. It has the content
> "elevator=noop". It doesn't seem too useful to me. But I'm fine with
> combination them.

I think they should be combined.  And that "elevator=noop" doesn't
sound unreasonable.

> > > It wasn't a problem for xl because the second extra= took precedence.
> > > However libvirt tests would only pick up the first extra= --  they
> > > worked by chance.
> > 
> > That's odd.  Is this to do with the xl -> libxl config converter ?
> > It seems to me that that converter should interpret xl config files
> > the same way xl does.  (Also xm did the same: last setting wins.)
> 
> Yes, the converter only picks up the first.

This is a bug, then.  Where should we file it ?

Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [OSSTEST PATCH 10/16] ts-debian-fixup: remove extra= before appending our own

[Wei Liu]

On Fri, Oct 20, 2017 at 12:03:24PM +0100, Ian Jackson wrote:
> Wei Liu writes ("[OSSTEST PATCH 10/16] ts-debian-fixup: remove extra= before 
> appending our own"):
> > The original extra= was not removed, so there were two extra= in the
> > resulting config file.
> 
> What is the original extra= ?  Why should we not combine them ?

The original extra= is generated by xen-create-image. It has the content
"elevator=noop". It doesn't seem too useful to me. But I'm fine with
combination them.

> 
> > It wasn't a problem for xl because the second extra= took precedence.
> > However libvirt tests would only pick up the first extra= --  they
> > worked by chance.
> 
> That's odd.  Is this to do with the xl -> libxl config converter ?
> It seems to me that that converter should interpret xl config files
> the same way xl does.  (Also xm did the same: last setting wins.)
> 

Yes, the converter only picks up the first.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [OSSTEST PATCH 10/16] ts-debian-fixup: remove extra= before appending our own

[Ian Jackson]

Wei Liu writes ("[OSSTEST PATCH 10/16] ts-debian-fixup: remove extra= before 
appending our own"):
> The original extra= was not removed, so there were two extra= in the
> resulting config file.

What is the original extra= ?  Why should we not combine them ?

> It wasn't a problem for xl because the second extra= took precedence.
> However libvirt tests would only pick up the first extra= --  they
> worked by chance.

That's odd.  Is this to do with the xl -> libxl config converter ?
It seems to me that that converter should interpret xl config files
the same way xl does.  (Also xm did the same: last setting wins.)

>  
> +
> +$cfg =~ s/^extra.*//mg;
>  $cfg .= "\nextra='$extra'\n";

Also, accidental whitespace change.

Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [OSSTEST PATCH 10/16] ts-debian-fixup: remove extra= before appending our own

[Wei Liu]

The original extra= was not removed, so there were two extra= in the
resulting config file.

It wasn't a problem for xl because the second extra= took precedence.
However libvirt tests would only pick up the first extra= --  they
worked by chance.

Fix this issue by removing the first extra=.

Signed-off-by: Wei Liu 
---
 ts-debian-fixup | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ts-debian-fixup b/ts-debian-fixup
index f29971d..d476467 100755
--- a/ts-debian-fixup
+++ b/ts-debian-fixup
@@ -175,6 +175,8 @@ sub otherfixupcfg () {
 $extra .= " iommu=soft";
 }
 
+
+$cfg =~ s/^extra.*//mg;
 $cfg .= "\nextra='$extra'\n";
 };
 
-- 
2.11.0


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel