| Message ID | 20171020103840.32762-11-wei.liu2@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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.
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.
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?
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"; };
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 <wei.liu2@citrix.com> --- ts-debian-fixup | 2 ++ 1 file changed, 2 insertions(+)