Clean up some duplicate code so it is easier to add new parameters. The tests need to be adjusted because after this patch there will be additional spaces at the end of lines in the output of virsh list, but this affects only the table output that is meant to be read by users. Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- tests/virshtest.c | 14 +++++++------- tools/virsh-domain-monitor.c | 38 +++++++++++++++++++------------------- 2 files changed, 26 insertions(+), 26 deletions(-) diff --git a/tests/virshtest.c b/tests/virshtest.c index 3fdae92b7834..5983b5b190d6 100644 --- a/tests/virshtest.c +++ b/tests/virshtest.c @@ -98,9 +98,9 @@ static int testCompareListDefault(const void *data ATTRIBUTE_UNUSED) { const char *const argv[] = { VIRSH_DEFAULT, "list", NULL }; const char *exp = "\ - Id Name State\n\ -----------------------------------------------------\n\ - 1 test running\n\ + Id Name State \n\ +-------------------------------------------------\n\ + 1 test running \n\ \n"; return testCompareOutputLit(exp, NULL, argv); } @@ -109,10 +109,10 @@ static int testCompareListCustom(const void *data ATTRIBUTE_UNUSED) { const char *const argv[] = { VIRSH_CUSTOM, "list", NULL }; const char *exp = "\ - Id Name State\n\ -----------------------------------------------------\n\ - 1 fv0 running\n\ - 2 fc4 running\n\ + Id Name State \n\ +-------------------------------------------------\n\ + 1 fv0 running \n\ + 2 fc4 running \n\ \n"; return testCompareOutputLit(exp, NULL, argv); } diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index db5bf4b2a566..24b902905968 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -1914,16 +1914,18 @@ cmdList(vshControl *ctl, const vshCmd *cmd) /* print table header in legacy mode */ if (optTable) { + vshPrintExtra(ctl, " %-5s %-30s %-10s", _("Id"), _("Name"), _("State")); + if (optTitle) - vshPrintExtra(ctl, " %-5s %-30s %-10s %-20s\n%s\n", - _("Id"), _("Name"), _("State"), _("Title"), - "-----------------------------------------" - "-----------------------------------------"); - else - vshPrintExtra(ctl, " %-5s %-30s %s\n%s\n", - _("Id"), _("Name"), _("State"), - "-----------------------------------------" - "-----------"); + vshPrintExtra(ctl, " %-20s\n", _("Title")); + + vshPrintExtra(ctl, "\n%s", + "-------------------------------------------------"); + + if (optTitle) + vshPrintExtra(ctl, "%s", "---------------------"); + + vshPrintExtra(ctl, "\n"); } for (i = 0; i < list->ndomains; i++) { @@ -1945,23 +1947,21 @@ cmdList(vshControl *ctl, const vshCmd *cmd) state = -2; if (optTable) { + vshPrint(ctl, " %-5s %-30s %-10s", id_buf, + virDomainGetName(dom), + state == -2 ? _("saved") + : virshDomainStateToString(state)); + if (optTitle) { if (!(title = virshGetDomainDescription(ctl, dom, true, 0))) goto cleanup; - vshPrint(ctl, " %-5s %-30s %-10s %-20s\n", id_buf, - virDomainGetName(dom), - state == -2 ? _("saved") - : virshDomainStateToString(state), - title); + vshPrint(ctl, " %-20s", title); VIR_FREE(title); - } else { - vshPrint(ctl, " %-5s %-30s %s\n", id_buf, - virDomainGetName(dom), - state == -2 ? _("saved") - : virshDomainStateToString(state)); } + + vshPrint(ctl, "\n"); } else if (optUUID) { if (virDomainGetUUIDString(dom, uuid) < 0) { vshError(ctl, "%s", _("Failed to get domain's UUID")); -- 2.6.3

On Thu, Dec 10, 2015 at 03:13:36PM -0500, John Ferlan wrote: I can do that. I don't like this output either, but I imagined increasing the complexity of the output generating would gain negative comments from others. It's not needed for this series, just a nice-to-have feature once this series is in (if ever). I'll fix that as a part of another round of rework. I'll generalize and unify it so that it looks nice most of the time. For the time being I did not talk at all about what non-ASCII characters do wit the output and I'm not going to deal with that, mainly because it' s aproblem of (f)printf itself. Not that I would see any adjustments coming, there wasn't mucha happening in this part of the code for some time, but I agree that if it can be done better I should try more ;)

On 12/01/2015 12:35 PM, Martin Kletzander wrote: Nice to have this test... Would be better to have one with Table too... Fairly easy to "dashcount += 10;" or because the longest reason I can find is 19 characters, perhaps use 20... The only oddity is long state reason when title is also used. Your call on this. not needing this if use dashcount... This should go after the Example since it seems that's related to the --title option... Might be nice to explain what a "state reason" is.... Maybe even provide another example with a couple of different state reasons for different States (eg, runnning, migrating, paused, etc states) ACK w/ a few adjustments. Your call on dashcount - I just thought it'd be easier. John

Create virDomainDefParseName that parses only the name from XML definition. This will be used in future patches. Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- src/conf/domain_conf.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d21e8cbd9d11..de6853a4dbd0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14446,6 +14446,19 @@ virDomainVcpuParse(virDomainDefPtr def, return ret; } +static int +virDomainDefParseName(virDomainDefPtr def, + xmlXPathContextPtr ctxt) +{ + if (!(def->name = virXPathString("string(./name[1])", ctxt))) { + virReportError(VIR_ERR_NO_NAME, NULL); + return -1; + } + + return 0; +} + + static virDomainDefPtr virDomainDefParseXML(xmlDocPtr xml, xmlNodePtr root, @@ -14571,11 +14584,8 @@ virDomainDefParseXML(xmlDocPtr xml, VIR_FREE(capsdata); } - /* Extract domain name */ - if (!(def->name = virXPathString("string(./name[1])", ctxt))) { - virReportError(VIR_ERR_NO_NAME, NULL); + if (virDomainDefParseName(def, ctxt) < 0) goto error; - } /* Extract domain uuid. If both uuid and sysinfo/system/entry/uuid * exist, they must match; and if only the latter exists, it can -- 2.6.3

On 12/01/2015 12:35 PM, Martin Kletzander wrote: since you're returning anyways... int ret; if ((ret = virUUIDParse(...)) < 0) virReportError(...) VIR_FREE(tmp); return ret; Your code works - IDC either way. if (virUUIDGenerate(...) < 0) ACK with at least this one fixed - just to conform with other error path uses... John

Add new parameter to virDomainObjListLoadConfig() and virDomainObjListLoadAllConfigs() that controls whether domains with invalid XML (which could not be parsed) should be kept in order not to lose track of them. For now, the parameter is set to false in all callers. Each driver can switch it to true when it is prepared to deal with such domains. For the domain object to be created add virDomainDefParseMinimal() that parses only name and UUID from the XML definition. UUID must be present, it will not be generated. The purpose of this function is to be used when all else fails, but we still want a domain object to work with. Also explicitly disable adding the invalid domain into the list of active ones, as that would render our internal structures inconsistent. Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- src/bhyve/bhyve_driver.c | 2 ++ src/conf/domain_conf.c | 36 +++++++++++++++++++++++ src/conf/domain_conf.h | 7 +++++ src/conf/virdomainobjlist.c | 71 ++++++++++++++++++++++++++++++++++++++++++--- src/conf/virdomainobjlist.h | 1 + src/libvirt_private.syms | 1 + src/libxl/libxl_driver.c | 3 ++ src/lxc/lxc_driver.c | 3 ++ src/qemu/qemu_driver.c | 3 ++ src/uml/uml_driver.c | 2 ++ 10 files changed, 125 insertions(+), 4 deletions(-) diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index fa1cdea24ae1..7f992dbfa8a1 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -1241,6 +1241,7 @@ bhyveStateInitialize(bool privileged, NULL, 1, bhyve_driver->caps, bhyve_driver->xmlopt, + false, NULL, NULL) < 0) goto cleanup; @@ -1249,6 +1250,7 @@ bhyveStateInitialize(bool privileged, BHYVE_AUTOSTART_DIR, 0, bhyve_driver->caps, bhyve_driver->xmlopt, + false, NULL, NULL) < 0) goto cleanup; diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7a295da507c4..0d9507eea186 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2472,6 +2472,9 @@ void virDomainDefFree(virDomainDefPtr def) xmlFreeNode(def->metadata); + VIR_FREE(def->xmlStr); + VIR_FREE(def->parseError); + VIR_FREE(def); } @@ -22529,6 +22532,39 @@ virDomainSaveStatus(virDomainXMLOptionPtr xmlopt, return ret; } +virDomainDefPtr +virDomainDefParseMinimal(const char *filename, + const char *xmlStr) +{ + xmlXPathContextPtr ctxt = NULL; + virDomainDefPtr def = NULL; + xmlDocPtr xml = NULL; + + if (!(xml = virXMLParse(filename, xmlStr, _("(domain_definition)")))) + goto cleanup; + + ctxt = xmlXPathNewContext(xml); + if (ctxt == NULL) { + virReportOOMError(); + goto cleanup; + } + + ctxt->node = xmlDocGetRootElement(xml); + + if (!(def = virDomainDefNew())) + goto cleanup; + + def->id = -1; + + if (virDomainDefParseName(def, ctxt) < 0 || + virDomainDefParseUUID(def, ctxt, true, NULL) < 0) + virDomainDefFree(def); + + cleanup: + xmlFreeDoc(xml); + xmlXPathFreeContext(ctxt); + return def; +} int virDomainDeleteConfig(const char *configDir, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 90d8e13638c2..3ff20205b4d6 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2215,6 +2215,10 @@ struct _virDomainDef { char *title; char *description; + /* Possible error and string that failed parsing */ + char *xmlStr; + char *parseError; + virDomainBlkiotune blkio; virDomainMemtune mem; @@ -2886,6 +2890,9 @@ int virDomainSaveStatus(virDomainXMLOptionPtr xmlopt, const char *statusDir, virDomainObjPtr obj) ATTRIBUTE_RETURN_CHECK; +virDomainDefPtr virDomainDefParseMinimal(const char *filename, + const char *xmlStr); + typedef void (*virDomainLoadConfigNotify)(virDomainObjPtr dom, int newDomain, void *opaque); diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index 7568f937420a..26366a05a150 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -254,6 +254,15 @@ virDomainObjListAddLocked(virDomainObjListPtr doms, } } + if (flags & VIR_DOMAIN_OBJ_LIST_ADD_LIVE && + vm->def->parseError) { + virReportError(VIR_ERR_XML_ERROR, + _("domain '%s' was not loaded due to an XML error " + "(%s), please redefine it"), + vm->def->name, vm->def->parseError); + goto error; + } + virDomainObjAssignDef(vm, def, !!(flags & VIR_DOMAIN_OBJ_LIST_ADD_LIVE), @@ -398,6 +407,7 @@ virDomainObjListLoadConfig(virDomainObjListPtr doms, const char *configDir, const char *autostartDir, const char *name, + bool keep_invalid, virDomainLoadConfigNotify notify, void *opaque) { @@ -406,13 +416,57 @@ virDomainObjListLoadConfig(virDomainObjListPtr doms, virDomainObjPtr dom; int autostart; virDomainDefPtr oldDef = NULL; + char *xmlStr = NULL; + char *xmlErr = NULL; if ((configFile = virDomainConfigFile(configDir, name)) == NULL) goto error; - if (!(def = virDomainDefParseFile(configFile, caps, xmlopt, - VIR_DOMAIN_DEF_PARSE_INACTIVE | - VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS))) - goto error; + + def = virDomainDefParseFile(configFile, caps, xmlopt, + VIR_DOMAIN_DEF_PARSE_INACTIVE | + VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS); + if (!def) { + char *tmp = NULL; + + if (!keep_invalid) + goto error; + + if (VIR_STRDUP(xmlErr, virGetLastErrorMessage()) < 0) + goto error; + + if (virFileReadAll(configFile, 1024*1024*10, &xmlStr) < 0) + goto error; + + if (!(def = virDomainDefParseMinimal(NULL, xmlStr))) + goto error; + + /* + * Remove the comment with a warning from the top. Don't fail + * if we can't copy it or find it. + */ + tmp = strstr(xmlStr, "-->"); + + if (tmp) + tmp += strlen("-->"); + else + tmp = xmlStr; + + if (virAsprintf(&def->xmlStr, + "<!-- %s\n\n%s\n-->%s", + _("WARNING: The following XML failed to load!" + "\n\n" + "In order for it to be loaded properly, " + "it needs to be fixed.\n" + "The error that was reported while loading " + "is provided below for your convenience:"), + xmlErr, tmp) < 0) { + def->xmlStr = xmlStr; + xmlStr = NULL; + } + + def->parseError = xmlErr; + xmlErr = NULL; + } if ((autostartLink = virDomainConfigFile(autostartDir, name)) == NULL) goto error; @@ -425,6 +479,11 @@ virDomainObjListLoadConfig(virDomainObjListPtr doms, dom->autostart = autostart; + if (def->parseError) { + virDomainObjSetState(dom, VIR_DOMAIN_SHUTOFF, + VIR_DOMAIN_SHUTOFF_INVALID_XML); + } + if (notify) (*notify)(dom, oldDef == NULL, opaque); @@ -434,6 +493,8 @@ virDomainObjListLoadConfig(virDomainObjListPtr doms, return dom; error: + VIR_FREE(xmlErr); + VIR_FREE(xmlStr); VIR_FREE(configFile); VIR_FREE(autostartLink); virDomainDefFree(def); @@ -505,6 +566,7 @@ virDomainObjListLoadAllConfigs(virDomainObjListPtr doms, int liveStatus, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, + bool keep_invalid, virDomainLoadConfigNotify notify, void *opaque) { @@ -552,6 +614,7 @@ virDomainObjListLoadAllConfigs(virDomainObjListPtr doms, configDir, autostartDir, entry->d_name, + keep_invalid, notify, opaque); if (dom) { diff --git a/src/conf/virdomainobjlist.h b/src/conf/virdomainobjlist.h index f47959825b10..fd5d6e461d3a 100644 --- a/src/conf/virdomainobjlist.h +++ b/src/conf/virdomainobjlist.h @@ -67,6 +67,7 @@ int virDomainObjListLoadAllConfigs(virDomainObjListPtr doms, int liveStatus, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, + bool keep_invalid, virDomainLoadConfigNotify notify, void *opaque); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index dd085c3487bf..702c360adede 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -225,6 +225,7 @@ virDomainDefNeedsPlacementAdvice; virDomainDefNew; virDomainDefNewFull; virDomainDefParseFile; +virDomainDefParseMinimal; virDomainDefParseNode; virDomainDefParseString; virDomainDefPostParse; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 35d7fae892a6..322be00eed42 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -696,6 +696,7 @@ libxlStateInitialize(bool privileged, 1, cfg->caps, libxl_driver->xmlopt, + false, NULL, NULL) < 0) goto error; @@ -708,6 +709,7 @@ libxlStateInitialize(bool privileged, 0, cfg->caps, libxl_driver->xmlopt, + false, NULL, NULL) < 0) goto error; @@ -748,6 +750,7 @@ libxlStateReload(void) 1, cfg->caps, libxl_driver->xmlopt, + false, NULL, libxl_driver); virDomainObjListForEach(libxl_driver->domains, libxlAutostartDomain, diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 1a9550e438ae..a4292046e486 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1666,6 +1666,7 @@ static int lxcStateInitialize(bool privileged, NULL, 1, caps, lxc_driver->xmlopt, + false, NULL, NULL) < 0) goto cleanup; @@ -1677,6 +1678,7 @@ static int lxcStateInitialize(bool privileged, cfg->autostartDir, 0, caps, lxc_driver->xmlopt, + false, NULL, NULL) < 0) goto cleanup; @@ -1741,6 +1743,7 @@ lxcStateReload(void) cfg->autostartDir, 0, caps, lxc_driver->xmlopt, + false, lxcNotifyLoadDomain, lxc_driver); virObjectUnref(caps); virObjectUnref(cfg); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 792b787659f4..e8711f0f3173 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -905,6 +905,7 @@ qemuStateInitialize(bool privileged, NULL, 1, qemu_driver->caps, qemu_driver->xmlopt, + false, NULL, NULL) < 0) goto error; @@ -927,6 +928,7 @@ qemuStateInitialize(bool privileged, cfg->autostartDir, 0, qemu_driver->caps, qemu_driver->xmlopt, + false, NULL, NULL) < 0) goto error; @@ -1007,6 +1009,7 @@ qemuStateReload(void) cfg->configDir, cfg->autostartDir, 0, caps, qemu_driver->xmlopt, + false, qemuNotifyLoadDomain, qemu_driver); cleanup: virObjectUnref(cfg); diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 14598fc20c8d..6136fe36b07c 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -578,6 +578,7 @@ umlStateInitialize(bool privileged, uml_driver->autostartDir, 0, uml_driver->caps, uml_driver->xmlopt, + false, NULL, NULL) < 0) goto error; @@ -646,6 +647,7 @@ umlStateReload(void) uml_driver->autostartDir, 0, uml_driver->caps, uml_driver->xmlopt, + false, umlNotifyLoadDomain, uml_driver); umlDriverUnlock(uml_driver); -- 2.6.3

On Thu, Dec 10, 2015 at 03:18:29PM -0500, John Ferlan wrote: This was a copy-paste from somewhere else, but I will add MAX_CONFIG_FILE_SIZE both here and to that copy-paste source as well (if I can find it ;)

Change qemuDomObjFromDomain() to qemuDomObjFromDomainInternal() with additional parameter that controls how to deal with invalid domains. New qemuDomObjFromDomain() then follows the safe path wo we don't have to change its callers from all APIs and qemuDomObjFromDomainInvalid() is added as a new function that can be called from APIs that are prepared to handle invalid definitions as well. Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e8711f0f3173..e89dfcd09d4a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -210,15 +210,22 @@ struct qemuAutostartData { /** * qemuDomObjFromDomain: * @domain: Domain pointer that has to be looked up + * @invalid: Whether to also return invalid definitions * * This function looks up @domain and returns the appropriate virDomainObjPtr * that has to be released by calling virDomainObjEndAPI(). * + * If @invalid is true, this function can return domain definition with only + * name and uuid set, so beware as that can lead to NULL pointer dereference. + * This function should be called with @invalid == true only from APIs that are + * needed to fix the domain definition (e.g. those needed for looking it up or + * providing a new definition. + * * Returns the domain object with incremented reference counter which is locked * on success, NULL otherwise. */ static virDomainObjPtr -qemuDomObjFromDomain(virDomainPtr domain) +qemuDomObjFromDomainInternal(virDomainPtr domain, bool invalid) { virDomainObjPtr vm; virQEMUDriverPtr driver = domain->conn->privateData; @@ -232,10 +239,29 @@ qemuDomObjFromDomain(virDomainPtr domain) uuidstr, domain->name); return NULL; } + if (!invalid && vm->def->parseError) { + virReportError(VIR_ERR_XML_ERROR, + _("domain '%s' was not loaded due to an XML error " + "(%s), please redefine it"), + vm->def->name, vm->def->parseError); + virDomainObjEndAPI(&vm); + } return vm; } +static inline virDomainObjPtr +qemuDomObjFromDomain(virDomainPtr domain) +{ + return qemuDomObjFromDomainInternal(domain, false); +} + +static inline virDomainObjPtr +qemuDomObjFromDomainInvalid(virDomainPtr domain) +{ + return qemuDomObjFromDomainInternal(domain, true); +} + /* Looks up the domain object from snapshot and unlocks the * driver. The returned domain object is locked and ref'd and the * caller must call virDomainObjEndAPI() on it. */ -- 2.6.3

If we load such domains, we don't need to handle invalid XML checking in qemuProcessStart, but we can move it to qemuDomainDefPostParse() or even into the parsing functions if all goes well. So the only thing we'll need to worry about after this is XML parsing code that would error out for running domains or non-qemu drivers. The latter will be fixed in following patches, hence making the active XML parsing the only problematic part. Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0f8a19836c43..e9cd7464f18d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -313,6 +313,7 @@ qemuAutostartDomain(virDomainObjPtr vm, virObjectRef(vm); virResetLastError(); if (vm->autostart && + !vm->def->parseError && !virDomainObjIsActive(vm)) { if (qemuProcessBeginJob(data->driver, vm) < 0) { err = virGetLastError(); @@ -954,7 +955,7 @@ qemuStateInitialize(bool privileged, cfg->autostartDir, 0, qemu_driver->caps, qemu_driver->xmlopt, - false, + true, NULL, NULL) < 0) goto error; @@ -1035,7 +1036,7 @@ qemuStateReload(void) cfg->configDir, cfg->autostartDir, 0, caps, qemu_driver->xmlopt, - false, + true, qemuNotifyLoadDomain, qemu_driver); cleanup: virObjectUnref(cfg); -- 2.6.3