7:47 a.m.
On 05/02/2018 01:24 PM, Prerna wrote:
On Sat, Apr 28, 2018 at 1:11 AM, John Ferlan <jferlan(a)redhat.com
<mailto:jferlan@redhat.com>> 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
<http://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 <encryption> and <auth> 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 <auth> and <encryption> into <source> from <disk>.
See
commits 37537a7c and 8002d3c which handle formatting as the XML was read.
Now if someone *changes* the read XML to use <source path.../>, then
that's a different story. But if you find:
<loader readonly='yes' secure='no'
type='rom'>/usr/lib/xen/boot/hvmloader</loader>
then you should format that.
If you find
<loader readonly='yes' secure='no' type='rom'>
<source file='/usr/lib/xen/boot/hvmloader'/>
</loader>
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 it?
Should this just be called "src" ? I was not sure if this sounded ambiguous.
Well, it's a "disk->src" - so there's precedence. I was merely
pointing
out loader->loader_src is redundant on loader.
> + virStorageSourceFree(loader->nvram);
> VIR_FREE(loader->templt);
> VIR_FREE(loader);
> }
[...]
> + loader->loader_src->type = VIR_STORAGE_TYPE_LAST;
ug, ah, no.
Consider my comment from above - you have either "path" or
"source" and
deal with it from there. When you format you need to format as you read.
As I mentioned above, I had understood that we deprecate the old format
in favour of the new. I also believe that the new is a superset of the old:
Eg, the old spec said :
<loader>/usr/share/OVMF/OVMF_CODE.fd</loader>
The new one should expect the same thing as:
<loader backing='file'><source
file='/usr/share/OVMF/OVMF_CODE.fd'/></loader>
So, if you notice, the two are not really distinct but rather the new
spec is a better description of the old.
So personally, I would prefer deprecating the old style format in favour
of new.
However, I'd go with community recommendation.
I don't disagree that "future" XML should use the newer format and can
be described that way in the docs (see how it was handled for the
commits I had).
Yes, it's not pleasant... But we can handle things internally different
than how they are presented/formatted. You can have a storage source as
the mechanism to manage internally and using a field in the internal
data structure to determine how the output will be formatted.
I do ask that you take the steps slowly when presenting patches.
Changing to store in storage source first and then adding in the other
storage source features later.
>
> readonly_str = virXMLPropString(node, "readonly");
> secure_str = virXMLPropString(node, "secure");
> type_str = virXMLPropString(node, "type");
> - loader->path = (char *) xmlNodeGetContent(node);
> +
[...]
> diff --git a/src/qemu/qemu_parse_command.c
b/src/qemu/qemu_parse_command.c
> index 0ce9632..2a0b200 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;
wow - changing this code too... not everyone does this! I really doubt
the code even really works very well any more.
Added for completeness. Pls let me know if you would want me to drop this.
Understood - it was a drive by comment ;-). Like I said, I'm not sure
how much of the command line parsing code really works right now.
There's so much new stuff that isn't included there. It'd be a "small
project" in order to get it back in line with what could be found on the
command line today.
>
> 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 @@
[...]
Perhaps best to generate a v2 with things separated better and more
complete changes and take it from there.
Agree with that. If only you could pls clarify whether we should
format-as-is-read or change-old-format-to-new.
Thanks once again for the review.
Regards,
Prerna
Well you have my opinion on the matter and an example. Whether anyone
else pipes in to state their opinion - who knows. Cannot force it. You
could wait a few days and then go from there or just start coding as I
suggested.
John