On Mon, Jun 4, 2018 at 6:24 PM, John Ferlan <jferlan@xxxxxxxxxx> wrote:
The $SUBJ and commit message are just poorly formatted.
On 05/21/2018 07:10 AM, Prerna Saxena wrote:
> 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.
>
> 2) Parse domain XML to generate virDomainLoaderDef & virDomainNvramDef.
>
> 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).
>
> 3) Format the loader source appropriately.
>
> 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.
>
> 4) Plumb the loader source into generation of QEMU command line.
>
> 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.
>
> 5) Fix the domain def inference logic to correctly account for network-backed
> pflash devices.
>
> 6) Bhyve: Fix command line generation to correctly pick up local loader path.
>
> 7) virt-aa-helper: Adjust references to loader & nvram elements to correctly
> parse the virStorageSource types.
>
> 8) Vbox: Adjust references to 'loader' and 'nvram' elements given that
> these are now represented by virStorageSourcePtr.
>
> 9)Xen: Adjust all references to loader & nvram elements given that they are now backed by virStorageSourcePtr
> ---
> src/bhyve/bhyve_command.c | 6 +-
> src/conf/domain_conf.c | 250 +++++++++++++++++++++++++++++++++++++---
> 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 | 54 ++++++---
> 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 +-
> 16 files changed, 409 insertions(+), 83 deletions(-)
>
But it pretty much shows that everything just got thrown into this one
patch and you went from there.
This series needs to progress a bit more "reasonably" to the desired
goal. Consider this progression (with the following as patch 1 of the
entire series):
1. Change path of loader to union:
struct _virDomainLoaderDef {
union {
char *path;
} loader;
then compile/fix up references.
2. Create an accessor to loader.path - that way future consumers can
just fetch the source path, e.g.:
const char *
virDomainLoaderDefGetLoaderPath(virDomainLoaderDefPtr loader)
{
return loader->loader.path;
}
Anything that accesses loader.path should change to use this via
something like:
const char *loaderPath;
...
loaderPath = virDomainLoaderDefGetLoaderPath(xxx)
...
Eventually this code will return either loader.path or loader.src->path
since both FILE and NETWORK storage use src->path.
3. Add virStorageSourcePtr to the loader union and modify places in the
code to use/read it. Update the domaincommon.rng, update formatdomain to
describe usage of <source> for <loader>, add genericxml2xmltests for
both "loader-source-file" and "loader-source-network" type formats for
at least "type='rom'". You could add type='pflash' tests too...
...
union {
char *path;
virStorageSourcePtr src;
} loader;
bool useLoaderSrc;
...
The patch code to parse needs to be changed to follow more closely what
virDomainDisk{BackingStore|DefMirror}Parse does... Alter ctxt locally
to the passed @node (and restore at the end). It should also be able to
use the union to do the magic, consider:
loader->loader.path = (char *) xmlNodeGetContent(node);
/* When not present, could return '' */
if (virStringIsEmpty(loader->loader.path))
VIR_FREE(loader->loader.path);
/* See if we need to look for new style <source...> subelement */
if (!loader->loader.path) {
xmlNodePtr source;
if (!(source = virXPathNode("./source", ctxt))) {
virReportError(VIR_ERR_XML_ERROR, "%s"
_("missing os loader source"));
goto cleanup;
}
if (VIR_ALLOC(loader->loader.src) < 0)
goto cleanup;
if ((tmp = virXMLPropString(source, "file")))
loader->loader.src->type = VIR_STORAGE_TYPE_FILE;
else if ((tmp = virXMLPropString(source, "protocol")))
loader->loader.src->type = VIR_STORAGE_TYPE_NETWORK;
if (loader->loader.src->type == VIR_STORAGE_TYPE_NONE) {
virReportError(VIR_ERR_XML_ERROR,
_("unknown loader source type: '%s"), tmp);
goto cleanup;
}
if (virDomainStorageSourceParse(source, ctxt,
loader->loader.src, 0) < 0)
goto cleanup;
loader->useLoaderSrc = true;
}
Then do the similar set of changes for nvram... Although for this one -
it's slightly trickier since <nvram> is optional... You'll probably
also need to modify qemuDomainDefPostParse to not automagically generate
an nvram.path if you're using <source>.
Once the xml2xml portion is done, the next patch alters qemu_command,
adds more tests, etc. Since you have generic xml2xml files, you probably
could just create a link from the qemuxml2argvdata directory or
create/use new files. Eventually it'd be nice for hte qemuxml2* code to
be able to use the generic xml files, but that's outside this scope.
BTW: I wouldn't bother with too many qemu_parse_command.c changes. Just
do the minimum. That code is so far out of date it's going to take a
solid effort to bring it back to today's options.
In any case, there's a lot of changes to be made so it's really not
worth going through each of the files in depth... I'll just point out
the domain_conf.h changes.
Thanks John for the elaborate review. I will start in this direction and post the next series asap.
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
