On Mon, Jun 4, 2018 at 6:24 PM, John Ferlan <jfer...@redhat.com> wrote:
>
> 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(-)
> >
>
> 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.
>
Thanks John for the elaborate review. I will start in this direction and
post the next series asap.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
