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

Reply via email to