<div dir="ltr"><div class="gmail_extra"> <div class="gmail_quote">On Mon, Jun 4, 2018 at 6:24 PM, John Ferlan <<a href="mailto:jferlan@redhat.com" target="_blank">jferlan@redhat.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"> 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(-) > </div></div>The $SUBJ and commit message are just poorly formatted. 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. </blockquote><div> </div><div>Thanks John for the elaborate review. I will start in this direction and post the next series asap.</div></div></div></div>