Re: [libvirt] [ v3 2/4] 1) Loader: Add a more elaborate definition.
On Mon, Jun 4, 2018 at 6:24 PM, John Ferlan 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:
> > /path/to/file
> >
> > we format it as was read.
> >
> > However, if it used new-style declaration:
> >
> >
> >
> >
> > 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 for , 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 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
Re: [libvirt] [ v3 2/4] 1) Loader: Add a more elaborate definition.
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:
> /path/to/file
>
> we format it as was read.
>
> However, if it used new-style declaration:
>
>
>
>
> 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 for , 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 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;
[libvirt] [ v3 2/4] 1) Loader: Add a more elaborate definition.
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:
/path/to/file
we format it as was read.
However, if it used new-style declaration:
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(-)
diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
index e3f7ded..9e53f40 100644
--- a/src/bhyve/bhyve_command.c
+++ b/src/bhyve/bhyve_command.c
@@ -521,10 +521,12 @@ virBhyveProcessBuildBhyveCmd(virConnectPtr conn,
virCommandAddArgList(cmd, "-s", "0:0,hostbridge", NULL);
if (def->os.bootloader == NULL &&
-def->os.loader) {
+def->os.loader &&
+def->os.loader->src &&
+def->os.loader->src->type == VIR_STORAGE_TYPE_FILE) {
if ((bhyveDriverGetCaps(conn) & BHYVE_CAP_LPC_BOOTROM)) {
virCommandAddArg(cmd, "-l");
-virCommandAddArgFormat(cmd, "bootrom,%s", def->os.loader->path);
+virCommandAddArgFormat(cmd, "bootrom,%s",
def->os.loader->src->path);
add_lpc = true;
} else {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 3689ac0..df2ed3a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2884,8 +2884,8 @@ virDomainLoaderDefFree(virDomainLoaderDefPtr loader)
if (!loader)
return;
-VIR_FREE(loader->path);
-VIR_FREE(loader->nvram);
+virStorageSourceFree(loader->src);
+virStorageSourceFree(loader->nvram);
VIR_FREE(loader->templt);
VIR_FREE(loader);
}
@@ -18087,30 +18087,81 @@
virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def)
static int
virDomainLoaderDefParseXML(xmlNodePtr node,
+ xmlXPathContextPtr ctxt,
virDomainLoaderDefPtr loader)
{
int ret = -1;
char *readonly_str = NULL;
char *secure_str = NULL;
char *type_str = NULL;
+char *tmp = NULL;
+char *lpath = NULL;
+xmlNodePtr cur;
+xmlXPathContextPtr cur_ctxt = ctxt;
+
+if (VIR_ALLOC(loader->src) < 0)
+goto error;
+
+loader->src->type = VIR_STORAGE_TYPE_LAST;
+loader->oldStyleLoader = false;
readonly_str = virXMLPropString(node, "readonly");
secure_str = virXMLPropString(node, "secure");
type_str = virXMLPropString(node, "type");
-loader->path = (char *) xmlNodeGetContent(node);
+
+if ((tmp = virXMLPropString(node, "backing")) &&
+(loader->src->type = virStorageTypeFromString(tmp)) <= 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unknown loader type '%s'"), tmp);
+VIR_FREE(tmp);
+goto error;
+}
+VIR_FREE(tmp);
+
+for (cur = node->children; cur != NULL; cur = cur->next) {
+if (cur->type != XML_ELEMENT_NODE)
+continue;
+
+if (virXMLNodeNameEqual(cur, "source")) {
+/* new-style declaration
