[libvirt] [PATCH v2 00/10] Make loading domains with invalid XML possible
We always had to deal with new parsing errors in a weird way. All of them needed to go into functions starting the domains. That messes up the code, it's confusing to newcomers and so on. I once had an idea that we can handle this stuff. We know what failed, we have the XML that failed parsing and if the problem is not in the domain name nor UUID, we can create a domain object out of that as well. This way we are able to do something we weren't with this series applied. Example follows. Assume "dummy" is a domain with invalid XML (I just modified the file). Now, just for the purpose of this silly example, let's say we allowed domains with archtecture x86_*, but now we've realized that x86_64 is the only one we want to allow, but there already is a domain defined that has <type arch='x86_256' .../>. With the current master, the domain would be lost, so we would need to modify the funstion starting the domain (e.g. qemuProcessStart for qemu domain). However, with this series, it behaves like this: # virsh list --all --reason Id Name State Reason --------------------------------------------------------------- - dummy shut off invalid XML # virsh domstate --reason dummy shut off (invalid XML) # virsh start dummy error: Failed to start domain dummy error: XML error: domain 'dummy' was not loaded due to an XML error (unsupported configuration: Unknown architecture x86_256), please redefine it # VISUAL='sed -i s/x86_256/x86_64/' virsh edit dummy Domain dummy XML configuration edited. # virsh domstate --reason dummy shut off (unknown) # virsh start dummy Domain dummy started This is a logical next step for us to clean and separate parsing and starting, getting rid of some old code without sacrifying compatibility and maybe generating parser code in the future. v2: - rebased on top of current master (with virdomainobjlist.c) - only disallow starting domains with invalid definitions (change done in v1), but allow re-defining them - add support for "virsh list --reason" to better excercise the feature added in this patchset v1: - rebase - don't allow starting domains with invalid state - https://www.redhat.com/archives/libvir-list/2015-November/msg01127.html RFC: https://www.redhat.com/archives/libvir-list/2015-September/msg00698.html Martin Kletzander (10): conf, virsh: Add new domain shutoff reason virsh: Refactor the table output of the list command virsh: Add support for list -reason qemu: Few whitespace cleanups conf: Extract name-parsing into its own function conf: Extract UUID parsing into its own function conf: Optionally keep domains with invalid XML, but don't allow starting them qemu: Don't lookup invalid domains unless specified otherwise qemu: Prepare basic APIs to handle invalid defs qemu: Load domains with invalid XML on start include/libvirt/libvirt-domain.h | 2 + src/bhyve/bhyve_driver.c | 2 + src/conf/domain_conf.c | 121 +++++++++++++++++++++++++++++++-------- 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 | 64 +++++++++++++++++---- src/uml/uml_driver.c | 2 + tests/virshtest.c | 30 +++++++--- tools/virsh-domain-monitor.c | 60 ++++++++++++------- tools/virsh.pod | 5 +- 14 files changed, 303 insertions(+), 69 deletions(-) -- 2.6.3
This new reason means the domain is shutoff because parsing of the XML failed while daemon was starting and from user's point of view, there's nothing else to do with it other than re-defining it. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- include/libvirt/libvirt-domain.h | 2 ++ src/conf/domain_conf.c | 3 ++- tools/virsh-domain-monitor.c | 3 ++- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index a1ea6a5d0786..37aa6db85cda 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -142,6 +142,8 @@ typedef enum { VIR_DOMAIN_SHUTOFF_FAILED = 6, /* domain failed to start */ VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT = 7, /* restored from a snapshot which was * taken while domain was shutoff */ + VIR_DOMAIN_SHUTOFF_INVALID_XML = 8, /* Domain XML failed to load for some + * reason, it needs to be re-defined */ # ifdef VIR_ENUM_SENTINELS VIR_DOMAIN_SHUTOFF_LAST # endif diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e6102a0b4254..d21e8cbd9d11 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -706,7 +706,8 @@ VIR_ENUM_IMPL(virDomainShutoffReason, VIR_DOMAIN_SHUTOFF_LAST, "migrated", "saved", "failed", - "from snapshot") + "from snapshot", + "invalid XML") VIR_ENUM_IMPL(virDomainCrashedReason, VIR_DOMAIN_CRASHED_LAST, "unknown", diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 64ec03dfc0cb..db5bf4b2a566 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -218,7 +218,8 @@ VIR_ENUM_IMPL(virshDomainShutoffReason, N_("migrated"), N_("saved"), N_("failed"), - N_("from snapshot")) + N_("from snapshot"), + N_("invalid XML")) VIR_ENUM_DECL(virshDomainCrashedReason) VIR_ENUM_IMPL(virshDomainCrashedReason, -- 2.6.3
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@redhat.com> --- 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 12/01/2015 12:35 PM, Martin Kletzander wrote:
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@redhat.com> --- 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\
[1]If only to make the output line up w/r/t \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"));
^^ I believe if Title were added there'd be an extra line once the following dashes are printed (end w/ \n, start with \n)
+ + vshPrintExtra(ctl, "\n%s", + "-------------------------------------------------"); + + if (optTitle) + vshPrintExtra(ctl, "%s", "---------------------");
It does seem in the expected virshtest.c output we're still off by 1. That is there is 1 more "-" than extra space after the State column. [1] Instead of counting dashes... dashcount = 45; /* Or some value - whatever it is */ ... if (optTitle) { ... dashcount += 20; } ,,, for (i = 0; i < dashcount; i++) buf[i] = '-'; vshPrintExtra(ctl, "%s\n", buf); ... where of course buf is appropriately defined ;-) A nit would be that the old output was 3 dashes longer. ACK with at least the fixup as a result of having Title on the line (and it would be nice to add a test for it too)! Whether you implement the '-' hack is up to you, but it may make future adjustments more simple... John
+ + 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"));
On Thu, Dec 10, 2015 at 03:13:36PM -0500, John Ferlan wrote:
On 12/01/2015 12:35 PM, Martin Kletzander wrote:
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@redhat.com> --- 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\
[1]If only to make the output line up w/r/t \n...
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).
\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"));
^^ I believe if Title were added there'd be an extra line once the following dashes are printed (end w/ \n, start with \n)
I'll fix that as a part of another round of rework.
+ + vshPrintExtra(ctl, "\n%s", + "-------------------------------------------------"); + + if (optTitle) + vshPrintExtra(ctl, "%s", "---------------------");
It does seem in the expected virshtest.c output we're still off by 1. That is there is 1 more "-" than extra space after the State column.
[1] Instead of counting dashes...
dashcount = 45; /* Or some value - whatever it is */ ... if (optTitle) { ... dashcount += 20; } ,,, for (i = 0; i < dashcount; i++) buf[i] = '-'; vshPrintExtra(ctl, "%s\n", buf); ...
where of course buf is appropriately defined ;-) A nit would be that the old output was 3 dashes longer.
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.
ACK with at least the fixup as a result of having Title on the line (and it would be nice to add a test for it too)!
Whether you implement the '-' hack is up to you, but it may make future adjustments more simple...
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 ;)
John
+ + 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"));
Add new parameter for the list command, --reason. This adds another optional column in the table output of listed domains. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- tests/virshtest.c | 16 ++++++++++++++++ tools/virsh-domain-monitor.c | 19 ++++++++++++++++++- tools/virsh.pod | 5 ++++- 3 files changed, 38 insertions(+), 2 deletions(-) diff --git a/tests/virshtest.c b/tests/virshtest.c index 5983b5b190d6..ecec898c7264 100644 --- a/tests/virshtest.c +++ b/tests/virshtest.c @@ -117,6 +117,18 @@ static int testCompareListCustom(const void *data ATTRIBUTE_UNUSED) return testCompareOutputLit(exp, NULL, argv); } +static int testCompareListReason(const void *data ATTRIBUTE_UNUSED) +{ + const char *const argv[] = { VIRSH_CUSTOM, "list", "--reason", NULL }; + const char *exp = "\ + Id Name State Reason \n\ +------------------------------------------------------------\n\ + 1 fv0 running unknown \n\ + 2 fc4 running unknown \n\ +\n"; + return testCompareOutputLit(exp, NULL, argv); +} + static int testCompareNodeinfoDefault(const void *data ATTRIBUTE_UNUSED) { const char *const argv[] = { VIRSH_DEFAULT, "nodeinfo", NULL }; @@ -266,6 +278,10 @@ mymain(void) testCompareListCustom, NULL) != 0) ret = -1; + if (virtTestRun("virsh list (with reason)", + testCompareListReason, NULL) != 0) + ret = -1; + if (virtTestRun("virsh nodeinfo (default)", testCompareNodeinfoDefault, NULL) != 0) ret = -1; diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 24b902905968..e2ef2c566f84 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -1848,6 +1848,10 @@ static const vshCmdOptDef opts_list[] = { .type = VSH_OT_BOOL, .help = N_("show domain title") }, + {.name = "reason", + .type = VSH_OT_BOOL, + .help = N_("show domain state reason") + }, {.name = NULL} }; @@ -1862,10 +1866,12 @@ cmdList(vshControl *ctl, const vshCmd *cmd) bool optTable = vshCommandOptBool(cmd, "table"); bool optUUID = vshCommandOptBool(cmd, "uuid"); bool optName = vshCommandOptBool(cmd, "name"); + bool optReason = vshCommandOptBool(cmd, "reason"); size_t i; char *title; char uuid[VIR_UUID_STRING_BUFLEN]; int state; + int state_reason; bool ret = false; virshDomainListPtr list = NULL; virDomainPtr dom; @@ -1916,12 +1922,18 @@ cmdList(vshControl *ctl, const vshCmd *cmd) if (optTable) { vshPrintExtra(ctl, " %-5s %-30s %-10s", _("Id"), _("Name"), _("State")); + if (optReason) + vshPrintExtra(ctl, " %-10s", _("Reason")); + if (optTitle) vshPrintExtra(ctl, " %-20s\n", _("Title")); vshPrintExtra(ctl, "\n%s", "-------------------------------------------------"); + if (optReason) + vshPrintExtra(ctl, "%s", "-----------"); + if (optTitle) vshPrintExtra(ctl, "%s", "---------------------"); @@ -1936,7 +1948,7 @@ cmdList(vshControl *ctl, const vshCmd *cmd) else ignore_value(virStrcpyStatic(id_buf, "-")); - state = virshDomainState(ctl, dom, NULL); + state = virshDomainState(ctl, dom, &state_reason); /* Domain could've been removed in the meantime */ if (state < 0) @@ -1952,6 +1964,11 @@ cmdList(vshControl *ctl, const vshCmd *cmd) state == -2 ? _("saved") : virshDomainStateToString(state)); + if (optReason) { + vshPrint(ctl, " %-10s", + virshDomainStateReasonToString(state, state_reason)); + } + if (optTitle) { if (!(title = virshGetDomainDescription(ctl, dom, true, 0))) goto cleanup; diff --git a/tools/virsh.pod b/tools/virsh.pod index 21ae4a3e5b46..7fddfc65d48c 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -393,7 +393,7 @@ value will generate output for the specific machine. Inject NMI to the guest. =item B<list> [I<--inactive> | I<--all>] - [I<--managed-save>] [I<--title>] + [I<--managed-save>] [I<--title>] [I<--reason>] { [I<--table>] | I<--name> | I<--uuid> } [I<--persistent>] [I<--transient>] [I<--with-managed-save>] [I<--without-managed-save>] @@ -531,6 +531,9 @@ If I<--title> is specified, then the short domain description (title) is printed in an extra column. This flag is usable only with the default I<--table> output. +If I<--reason> is specified, then the state reason is printed in an extra +column. This flag is usable only with the default I<--table> output. + Example: B<virsh> list --title -- 2.6.3
On 12/01/2015 12:35 PM, Martin Kletzander wrote:
Add new parameter for the list command, --reason. This adds another optional column in the table output of listed domains.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- tests/virshtest.c | 16 ++++++++++++++++ tools/virsh-domain-monitor.c | 19 ++++++++++++++++++- tools/virsh.pod | 5 ++++- 3 files changed, 38 insertions(+), 2 deletions(-)
diff --git a/tests/virshtest.c b/tests/virshtest.c index 5983b5b190d6..ecec898c7264 100644 --- a/tests/virshtest.c +++ b/tests/virshtest.c @@ -117,6 +117,18 @@ static int testCompareListCustom(const void *data ATTRIBUTE_UNUSED) return testCompareOutputLit(exp, NULL, argv); }
+static int testCompareListReason(const void *data ATTRIBUTE_UNUSED) +{ + const char *const argv[] = { VIRSH_CUSTOM, "list", "--reason", NULL }; + const char *exp = "\ + Id Name State Reason \n\ +------------------------------------------------------------\n\ + 1 fv0 running unknown \n\ + 2 fc4 running unknown \n\ +\n"; + return testCompareOutputLit(exp, NULL, argv); +} +
Nice to have this test... Would be better to have one with Table too...
static int testCompareNodeinfoDefault(const void *data ATTRIBUTE_UNUSED) { const char *const argv[] = { VIRSH_DEFAULT, "nodeinfo", NULL }; @@ -266,6 +278,10 @@ mymain(void) testCompareListCustom, NULL) != 0) ret = -1;
+ if (virtTestRun("virsh list (with reason)", + testCompareListReason, NULL) != 0) + ret = -1; + if (virtTestRun("virsh nodeinfo (default)", testCompareNodeinfoDefault, NULL) != 0) ret = -1; diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 24b902905968..e2ef2c566f84 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -1848,6 +1848,10 @@ static const vshCmdOptDef opts_list[] = { .type = VSH_OT_BOOL, .help = N_("show domain title") }, + {.name = "reason", + .type = VSH_OT_BOOL, + .help = N_("show domain state reason") + }, {.name = NULL} };
@@ -1862,10 +1866,12 @@ cmdList(vshControl *ctl, const vshCmd *cmd) bool optTable = vshCommandOptBool(cmd, "table"); bool optUUID = vshCommandOptBool(cmd, "uuid"); bool optName = vshCommandOptBool(cmd, "name"); + bool optReason = vshCommandOptBool(cmd, "reason"); size_t i; char *title; char uuid[VIR_UUID_STRING_BUFLEN]; int state; + int state_reason; bool ret = false; virshDomainListPtr list = NULL; virDomainPtr dom; @@ -1916,12 +1922,18 @@ cmdList(vshControl *ctl, const vshCmd *cmd) if (optTable) { vshPrintExtra(ctl, " %-5s %-30s %-10s", _("Id"), _("Name"), _("State"));
+ if (optReason) + vshPrintExtra(ctl, " %-10s", _("Reason")); +
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.
if (optTitle) vshPrintExtra(ctl, " %-20s\n", _("Title"));
vshPrintExtra(ctl, "\n%s", "-------------------------------------------------");
+ if (optReason) + vshPrintExtra(ctl, "%s", "-----------"); +
not needing this if use dashcount...
if (optTitle) vshPrintExtra(ctl, "%s", "---------------------");
@@ -1936,7 +1948,7 @@ cmdList(vshControl *ctl, const vshCmd *cmd) else ignore_value(virStrcpyStatic(id_buf, "-"));
- state = virshDomainState(ctl, dom, NULL); + state = virshDomainState(ctl, dom, &state_reason);
/* Domain could've been removed in the meantime */ if (state < 0) @@ -1952,6 +1964,11 @@ cmdList(vshControl *ctl, const vshCmd *cmd) state == -2 ? _("saved") : virshDomainStateToString(state));
+ if (optReason) { + vshPrint(ctl, " %-10s", + virshDomainStateReasonToString(state, state_reason)); + } + if (optTitle) { if (!(title = virshGetDomainDescription(ctl, dom, true, 0))) goto cleanup; diff --git a/tools/virsh.pod b/tools/virsh.pod index 21ae4a3e5b46..7fddfc65d48c 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -393,7 +393,7 @@ value will generate output for the specific machine. Inject NMI to the guest.
=item B<list> [I<--inactive> | I<--all>] - [I<--managed-save>] [I<--title>] + [I<--managed-save>] [I<--title>] [I<--reason>] { [I<--table>] | I<--name> | I<--uuid> } [I<--persistent>] [I<--transient>] [I<--with-managed-save>] [I<--without-managed-save>] @@ -531,6 +531,9 @@ If I<--title> is specified, then the short domain description (title) is printed in an extra column. This flag is usable only with the default I<--table> output.
+If I<--reason> is specified, then the state reason is printed in an extra +column. This flag is usable only with the default I<--table> output. +
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
Example:
B<virsh> list --title
On Thu, Dec 10, 2015 at 03:15:43PM -0500, John Ferlan wrote:
On 12/01/2015 12:35 PM, Martin Kletzander wrote:
Add new parameter for the list command, --reason. This adds another optional column in the table output of listed domains.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- tests/virshtest.c | 16 ++++++++++++++++ tools/virsh-domain-monitor.c | 19 ++++++++++++++++++- tools/virsh.pod | 5 ++++- 3 files changed, 38 insertions(+), 2 deletions(-)
diff --git a/tests/virshtest.c b/tests/virshtest.c index 5983b5b190d6..ecec898c7264 100644 --- a/tests/virshtest.c +++ b/tests/virshtest.c @@ -117,6 +117,18 @@ static int testCompareListCustom(const void *data ATTRIBUTE_UNUSED) return testCompareOutputLit(exp, NULL, argv); }
+static int testCompareListReason(const void *data ATTRIBUTE_UNUSED) +{ + const char *const argv[] = { VIRSH_CUSTOM, "list", "--reason", NULL }; + const char *exp = "\ + Id Name State Reason \n\ +------------------------------------------------------------\n\ + 1 fv0 running unknown \n\ + 2 fc4 running unknown \n\ +\n"; + return testCompareOutputLit(exp, NULL, argv); +} +
Nice to have this test... Would be better to have one with Table too...
You mean Title, not Table, right? This is Table output (default). With Title I would find out that there is extra newline, I'll include that as well.
static int testCompareNodeinfoDefault(const void *data ATTRIBUTE_UNUSED) { const char *const argv[] = { VIRSH_DEFAULT, "nodeinfo", NULL }; @@ -266,6 +278,10 @@ mymain(void) testCompareListCustom, NULL) != 0) ret = -1;
+ if (virtTestRun("virsh list (with reason)", + testCompareListReason, NULL) != 0) + ret = -1; + if (virtTestRun("virsh nodeinfo (default)", testCompareNodeinfoDefault, NULL) != 0) ret = -1; diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 24b902905968..e2ef2c566f84 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -1848,6 +1848,10 @@ static const vshCmdOptDef opts_list[] = { .type = VSH_OT_BOOL, .help = N_("show domain title") }, + {.name = "reason", + .type = VSH_OT_BOOL, + .help = N_("show domain state reason") + }, {.name = NULL} };
@@ -1862,10 +1866,12 @@ cmdList(vshControl *ctl, const vshCmd *cmd) bool optTable = vshCommandOptBool(cmd, "table"); bool optUUID = vshCommandOptBool(cmd, "uuid"); bool optName = vshCommandOptBool(cmd, "name"); + bool optReason = vshCommandOptBool(cmd, "reason"); size_t i; char *title; char uuid[VIR_UUID_STRING_BUFLEN]; int state; + int state_reason; bool ret = false; virshDomainListPtr list = NULL; virDomainPtr dom; @@ -1916,12 +1922,18 @@ cmdList(vshControl *ctl, const vshCmd *cmd) if (optTable) { vshPrintExtra(ctl, " %-5s %-30s %-10s", _("Id"), _("Name"), _("State"));
+ if (optReason) + vshPrintExtra(ctl, " %-10s", _("Reason")); +
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.
if (optTitle) vshPrintExtra(ctl, " %-20s\n", _("Title"));
vshPrintExtra(ctl, "\n%s", "-------------------------------------------------");
+ if (optReason) + vshPrintExtra(ctl, "%s", "-----------"); +
not needing this if use dashcount...
if (optTitle) vshPrintExtra(ctl, "%s", "---------------------");
@@ -1936,7 +1948,7 @@ cmdList(vshControl *ctl, const vshCmd *cmd) else ignore_value(virStrcpyStatic(id_buf, "-"));
- state = virshDomainState(ctl, dom, NULL); + state = virshDomainState(ctl, dom, &state_reason);
/* Domain could've been removed in the meantime */ if (state < 0) @@ -1952,6 +1964,11 @@ cmdList(vshControl *ctl, const vshCmd *cmd) state == -2 ? _("saved") : virshDomainStateToString(state));
+ if (optReason) { + vshPrint(ctl, " %-10s", + virshDomainStateReasonToString(state, state_reason)); + } + if (optTitle) { if (!(title = virshGetDomainDescription(ctl, dom, true, 0))) goto cleanup; diff --git a/tools/virsh.pod b/tools/virsh.pod index 21ae4a3e5b46..7fddfc65d48c 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -393,7 +393,7 @@ value will generate output for the specific machine. Inject NMI to the guest.
=item B<list> [I<--inactive> | I<--all>] - [I<--managed-save>] [I<--title>] + [I<--managed-save>] [I<--title>] [I<--reason>] { [I<--table>] | I<--name> | I<--uuid> } [I<--persistent>] [I<--transient>] [I<--with-managed-save>] [I<--without-managed-save>] @@ -531,6 +531,9 @@ If I<--title> is specified, then the short domain description (title) is printed in an extra column. This flag is usable only with the default I<--table> output.
+If I<--reason> is specified, then the state reason is printed in an extra +column. This flag is usable only with the default I<--table> output. +
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
Example:
B<virsh> list --title
On 12/11/2015 05:40 AM, Martin Kletzander wrote: [...]
+static int testCompareListReason(const void *data ATTRIBUTE_UNUSED) +{ + const char *const argv[] = { VIRSH_CUSTOM, "list", "--reason", NULL }; + const char *exp = "\ + Id Name State Reason \n\ +------------------------------------------------------------\n\ + 1 fv0 running unknown \n\ + 2 fc4 running unknown \n\ +\n"; + return testCompareOutputLit(exp, NULL, argv); +} +
Nice to have this test... Would be better to have one with Table too...
You mean Title, not Table, right? This is Table output (default). With Title I would find out that there is extra newline, I'll include that as well.
Yeah Title not Table John [...]
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ae1d8e7ab397..792b787659f4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7047,9 +7047,9 @@ qemuDomainObjRestore(virConnectPtr conn, } -static char -*qemuDomainGetXMLDesc(virDomainPtr dom, - unsigned int flags) +static char * +qemuDomainGetXMLDesc(virDomainPtr dom, + unsigned int flags) { virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; @@ -7456,7 +7456,10 @@ qemuDomainCreate(virDomainPtr dom) return qemuDomainCreateWithFlags(dom, 0); } -static virDomainPtr qemuDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) +static virDomainPtr +qemuDomainDefineXMLFlags(virConnectPtr conn, + const char *xml, + unsigned int flags) { virQEMUDriverPtr driver = conn->privateData; virDomainDefPtr def = NULL; -- 2.6.3
Create virDomainDefParseName that parses only the name from XML definition. This will be used in future patches. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- 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
Create virDomainDefParseUUID that parses only the UUID from XML definition. This will be used in future patch(es). Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/conf/domain_conf.c | 64 +++++++++++++++++++++++++++++++++++--------------- 1 file changed, 45 insertions(+), 19 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index de6853a4dbd0..7a295da507c4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14446,6 +14446,49 @@ virDomainVcpuParse(virDomainDefPtr def, return ret; } +/* + * Parse domain's UUID, optionally generating it if missing. + */ +static int +virDomainDefParseUUID(virDomainDefPtr def, + xmlXPathContextPtr ctxt, + bool required, + bool *generated) +{ + char *tmp = virXPathString("string(./uuid[1])", ctxt); + + if (generated) + *generated = false; + + if (tmp) { + if (virUUIDParse(tmp, def->uuid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("malformed uuid element")); + VIR_FREE(tmp); + return -1; + } + VIR_FREE(tmp); + return 0; + } + + if (required) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing required UUID")); + return -1; + } + + if (virUUIDGenerate(def->uuid)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Failed to generate UUID")); + return -1; + } + + if (generated) + *generated = true; + + return 0; +} + static int virDomainDefParseName(virDomainDefPtr def, xmlXPathContextPtr ctxt) @@ -14587,25 +14630,8 @@ virDomainDefParseXML(xmlDocPtr xml, 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 - * also serve as the uuid. */ - tmp = virXPathString("string(./uuid[1])", ctxt); - if (!tmp) { - if (virUUIDGenerate(def->uuid)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Failed to generate UUID")); - goto error; - } - uuid_generated = true; - } else { - if (virUUIDParse(tmp, def->uuid) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("malformed uuid element")); - goto error; - } - VIR_FREE(tmp); - } + if (virDomainDefParseUUID(def, ctxt, false, &uuid_generated) < 0) + goto error; /* Extract short description of domain (title) */ def->title = virXPathString("string(./title[1])", ctxt); -- 2.6.3
On 12/01/2015 12:35 PM, Martin Kletzander wrote:
Create virDomainDefParseUUID that parses only the UUID from XML definition. This will be used in future patch(es).
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/conf/domain_conf.c | 64 +++++++++++++++++++++++++++++++++++--------------- 1 file changed, 45 insertions(+), 19 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index de6853a4dbd0..7a295da507c4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14446,6 +14446,49 @@ virDomainVcpuParse(virDomainDefPtr def, return ret; }
+/* + * Parse domain's UUID, optionally generating it if missing. + */ +static int +virDomainDefParseUUID(virDomainDefPtr def, + xmlXPathContextPtr ctxt, + bool required, + bool *generated) +{ + char *tmp = virXPathString("string(./uuid[1])", ctxt); + + if (generated) + *generated = false; + + if (tmp) { + if (virUUIDParse(tmp, def->uuid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("malformed uuid element")); + VIR_FREE(tmp); + return -1; + } + VIR_FREE(tmp); + return 0;
since you're returning anyways... int ret; if ((ret = virUUIDParse(...)) < 0) virReportError(...) VIR_FREE(tmp); return ret; Your code works - IDC either way.
+ } + + if (required) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing required UUID")); + return -1; + } + + if (virUUIDGenerate(def->uuid)) {
if (virUUIDGenerate(...) < 0) ACK with at least this one fixed - just to conform with other error path uses... John
+ virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Failed to generate UUID")); + return -1; + } + + if (generated) + *generated = true; + + return 0; +} + static int virDomainDefParseName(virDomainDefPtr def, xmlXPathContextPtr ctxt) @@ -14587,25 +14630,8 @@ virDomainDefParseXML(xmlDocPtr xml, 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 - * also serve as the uuid. */ - tmp = virXPathString("string(./uuid[1])", ctxt); - if (!tmp) { - if (virUUIDGenerate(def->uuid)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Failed to generate UUID")); - goto error; - } - uuid_generated = true; - } else { - if (virUUIDParse(tmp, def->uuid) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("malformed uuid element")); - goto error; - } - VIR_FREE(tmp); - } + if (virDomainDefParseUUID(def, ctxt, false, &uuid_generated) < 0) + goto error;
/* Extract short description of domain (title) */ def->title = virXPathString("string(./title[1])", ctxt);
On Thu, Dec 10, 2015 at 03:17:01PM -0500, John Ferlan wrote:
On 12/01/2015 12:35 PM, Martin Kletzander wrote:
Create virDomainDefParseUUID that parses only the UUID from XML definition. This will be used in future patch(es).
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/conf/domain_conf.c | 64 +++++++++++++++++++++++++++++++++++--------------- 1 file changed, 45 insertions(+), 19 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index de6853a4dbd0..7a295da507c4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14446,6 +14446,49 @@ virDomainVcpuParse(virDomainDefPtr def, return ret; }
+/* + * Parse domain's UUID, optionally generating it if missing. + */ +static int +virDomainDefParseUUID(virDomainDefPtr def, + xmlXPathContextPtr ctxt, + bool required, + bool *generated) +{ + char *tmp = virXPathString("string(./uuid[1])", ctxt); + + if (generated) + *generated = false; + + if (tmp) { + if (virUUIDParse(tmp, def->uuid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("malformed uuid element")); + VIR_FREE(tmp); + return -1; + } + VIR_FREE(tmp); + return 0;
since you're returning anyways...
int ret; if ((ret = virUUIDParse(...)) < 0) virReportError(...) VIR_FREE(tmp); return ret;
Your code works - IDC either way.
I like your way a bit more.
+ } + + if (required) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing required UUID")); + return -1; + } + + if (virUUIDGenerate(def->uuid)) {
if (virUUIDGenerate(...) < 0)
And this one is just my fault, of course it needs to be "< 0".
ACK with at least this one fixed - just to conform with other error path uses...
John
+ virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Failed to generate UUID")); + return -1; + } + + if (generated) + *generated = true; + + return 0; +} + static int virDomainDefParseName(virDomainDefPtr def, xmlXPathContextPtr ctxt) @@ -14587,25 +14630,8 @@ virDomainDefParseXML(xmlDocPtr xml, 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 - * also serve as the uuid. */ - tmp = virXPathString("string(./uuid[1])", ctxt); - if (!tmp) { - if (virUUIDGenerate(def->uuid)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Failed to generate UUID")); - goto error; - } - uuid_generated = true; - } else { - if (virUUIDParse(tmp, def->uuid) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("malformed uuid element")); - goto error; - } - VIR_FREE(tmp); - } + if (virDomainDefParseUUID(def, ctxt, false, &uuid_generated) < 0) + goto error;
/* Extract short description of domain (title) */ def->title = virXPathString("string(./title[1])", ctxt);
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@redhat.com> --- 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 12/01/2015 12:35 PM, Martin Kletzander wrote:
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@redhat.com> --- 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(-)
[...]
@@ -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)
Any reason to not use MAX_CONFIG_FILE_SIZE? Cannot imagine this failing, but I mention for consistency ACK John
+ goto error; + + if (!(def = virDomainDefParseMinimal(NULL, xmlStr))) + goto error; +
[...]
On Thu, Dec 10, 2015 at 03:18:29PM -0500, John Ferlan wrote:
On 12/01/2015 12:35 PM, Martin Kletzander wrote:
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@redhat.com> --- 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(-)
[...]
@@ -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)
Any reason to not use MAX_CONFIG_FILE_SIZE? Cannot imagine this failing, but I mention for consistency
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 ;)
ACK
John
+ goto error; + + if (!(def = virDomainDefParseMinimal(NULL, xmlStr))) + goto error; +
[...]
[...]
+ + if (virFileReadAll(configFile, 1024*1024*10, &xmlStr) < 0)
Any reason to not use MAX_CONFIG_FILE_SIZE? Cannot imagine this failing, but I mention for consistency
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 ;)
Dang it's in a .c file now... I initially cscope searched on virFileReadAll... That had some examples... Which is why I mentioned it. Using 'find src -exec grep "1024.*\*1024.*\*10" {} \;' yields src/util/virconf.c src/security/security_apparmor.h Couldn't say the magic words to cscope to take that string and find anything though ;-) John
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@redhat.com> --- 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
In order for the user to be able to fix broken domains function qemuDomainGetXMLDesc() needs to be able to lookup invalid domain definitions and handle them properly. When redefined, function qemuDomainDefineXMLFlags() must clear the 'invalid XML' reason. As a nice addition, qemuDomainGetState() can lookup such domains without any other change and that allows virsh not only to get their status, but also to list them. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e89dfcd09d4a..0f8a19836c43 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2711,7 +2711,7 @@ qemuDomainGetState(virDomainPtr dom, virCheckFlags(0, -1); - if (!(vm = qemuDomObjFromDomain(dom))) + if (!(vm = qemuDomObjFromDomainInvalid(dom))) goto cleanup; if (virDomainGetStateEnsureACL(dom->conn, vm->def) < 0) @@ -7086,19 +7086,23 @@ qemuDomainGetXMLDesc(virDomainPtr dom, /* Flags checked by virDomainDefFormat */ - if (!(vm = qemuDomObjFromDomain(dom))) + if (!(vm = qemuDomObjFromDomainInvalid(dom))) goto cleanup; if (virDomainGetXMLDescEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; - if (qemuDomainUpdateCurrentMemorySize(driver, vm) < 0) - goto cleanup; + if (vm->def->parseError) { + ignore_value(VIR_STRDUP(ret, vm->def->xmlStr)); + } else { + if (qemuDomainUpdateCurrentMemorySize(driver, vm) < 0) + goto cleanup; - if ((flags & VIR_DOMAIN_XML_MIGRATABLE)) - flags |= QEMU_DOMAIN_FORMAT_LIVE_FLAGS; + if ((flags & VIR_DOMAIN_XML_MIGRATABLE)) + flags |= QEMU_DOMAIN_FORMAT_LIVE_FLAGS; - ret = qemuDomainFormatXML(driver, vm, flags); + ret = qemuDomainFormatXML(driver, vm, flags); + } cleanup: virDomainObjEndAPI(&vm); @@ -7566,6 +7570,9 @@ qemuDomainDefineXMLFlags(virConnectPtr conn, goto cleanup; } + if (oldDef && oldDef->parseError) + virDomainObjSetState(vm, virDomainObjGetState(vm, NULL), 0); + event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_DEFINED, !oldDef ? -- 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@redhat.com> --- 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
On 12/02/2015 01:35 AM, Martin Kletzander wrote:
We always had to deal with new parsing errors in a weird way. All of them needed to go into functions starting the domains. That messes up the code, it's confusing to newcomers and so on.
I once had an idea that we can handle this stuff. We know what failed, we have the XML that failed parsing and if the problem is not in the domain name nor UUID, we can create a domain object out of that as well. This way we are able to do something we weren't with this series applied. Example follows.
Assume "dummy" is a domain with invalid XML (I just modified the file). Now, just for the purpose of this silly example, let's say we allowed domains with archtecture x86_*, but now we've realized that x86_64 is the only one we want to allow, but there already is a domain defined that has <type arch='x86_256' .../>. With the current master, the domain would be lost, so we would need to modify the funstion starting the domain (e.g. qemuProcessStart for qemu domain). However, with this series, it behaves like this:
# virsh list --all --reason Id Name State Reason --------------------------------------------------------------- - dummy shut off invalid XML
# virsh domstate --reason dummy shut off (invalid XML)
# virsh start dummy error: Failed to start domain dummy error: XML error: domain 'dummy' was not loaded due to an XML error (unsupported configuration: Unknown architecture x86_256), please redefine it
# VISUAL='sed -i s/x86_256/x86_64/' virsh edit dummy Domain dummy XML configuration edited.
# virsh domstate --reason dummy shut off (unknown)
# virsh start dummy Domain dummy started
This is a logical next step for us to clean and separate parsing and starting, getting rid of some old code without sacrifying compatibility and maybe generating parser code in the future.
v2: - rebased on top of current master (with virdomainobjlist.c) - only disallow starting domains with invalid definitions (change done in v1), but allow re-defining them - add support for "virsh list --reason" to better excercise the feature added in this patchset
v1: - rebase - don't allow starting domains with invalid state - https://www.redhat.com/archives/libvir-list/2015-November/msg01127.html
RFC: https://www.redhat.com/archives/libvir-list/2015-September/msg00698.html
Martin Kletzander (10): conf, virsh: Add new domain shutoff reason virsh: Refactor the table output of the list command virsh: Add support for list -reason qemu: Few whitespace cleanups conf: Extract name-parsing into its own function conf: Extract UUID parsing into its own function conf: Optionally keep domains with invalid XML, but don't allow starting them qemu: Don't lookup invalid domains unless specified otherwise qemu: Prepare basic APIs to handle invalid defs qemu: Load domains with invalid XML on start
include/libvirt/libvirt-domain.h | 2 + src/bhyve/bhyve_driver.c | 2 + src/conf/domain_conf.c | 121 +++++++++++++++++++++++++++++++-------- 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 | 64 +++++++++++++++++---- src/uml/uml_driver.c | 2 + tests/virshtest.c | 30 +++++++--- tools/virsh-domain-monitor.c | 60 ++++++++++++------- tools/virsh.pod | 5 +- 14 files changed, 303 insertions(+), 69 deletions(-)
I am glad to see these patches you told me before. And i have test your patches and found some problem until now: 1. if guest xml have private info, libvirt will output it if xml is invalid: # virsh list --all --reason Id Name State Reason ------------------------------------------------------------ - test3 shut off invalid XML # virsh -r dumpxml test3 ... <input type='keyboard' bus='ps3'/> <----wrong place <graphics type='vnc' port='-1' autoport='yes' listen='127.0.0.1' passwd='123'> <----show passwd 2. vcpucount command output looks strange (small problem :) ): # virsh vcpucount test3 3. hit crash if the guest xml uuid is invalid (delete a number) during stop daemon: Program terminated with signal 11, Segmentation fault. #0 0x00007fcbe773af87 in virDomainDefFree (def=0x7fcbc423d980) at conf/domain_conf.c:2327 2327 virDomainGraphicsDefFree(def->graphics[i]); Thread 1 (Thread 0x7fcbe835c8c0 (LWP 21589)): #0 0x00007fcbe773af87 in virDomainDefFree (def=0x7fcbc423d980) at conf/domain_conf.c:2327 #1 0x00007fcbe773b903 in virDomainObjDispose (obj=0x7fcbc4235fe0) at conf/domain_conf.c:2487 #2 0x00007fcbe76f182b in virObjectUnref (anyobj=<optimized out>) at util/virobject.c:265 #3 0x00007fcbe76d0ac9 in virHashFree (table=0x7fcbc412eda0) at util/virhash.c:318 #4 0x00007fcbe76f182b in virObjectUnref (anyobj=<optimized out>) at util/virobject.c:265 #5 0x00007fcbce85f8cf in qemuStateCleanup () at qemu/qemu_driver.c:1126 #6 0x00007fcbe779a168 in virStateCleanup () at libvirt.c:815 #7 0x00007fcbe83f02b1 in main (argc=<optimized out>, argv=<optimized out>) at libvirtd.c:1619 Thanks, Luyao
On Wed, Dec 02, 2015 at 04:46:20PM +0800, lhuang wrote:
On 12/02/2015 01:35 AM, Martin Kletzander wrote:
We always had to deal with new parsing errors in a weird way. All of them needed to go into functions starting the domains. That messes up the code, it's confusing to newcomers and so on.
I once had an idea that we can handle this stuff. We know what failed, we have the XML that failed parsing and if the problem is not in the domain name nor UUID, we can create a domain object out of that as well. This way we are able to do something we weren't with this series applied. Example follows.
Assume "dummy" is a domain with invalid XML (I just modified the file). Now, just for the purpose of this silly example, let's say we allowed domains with archtecture x86_*, but now we've realized that x86_64 is the only one we want to allow, but there already is a domain defined that has <type arch='x86_256' .../>. With the current master, the domain would be lost, so we would need to modify the funstion starting the domain (e.g. qemuProcessStart for qemu domain). However, with this series, it behaves like this:
# virsh list --all --reason Id Name State Reason --------------------------------------------------------------- - dummy shut off invalid XML
# virsh domstate --reason dummy shut off (invalid XML)
# virsh start dummy error: Failed to start domain dummy error: XML error: domain 'dummy' was not loaded due to an XML error (unsupported configuration: Unknown architecture x86_256), please redefine it
# VISUAL='sed -i s/x86_256/x86_64/' virsh edit dummy Domain dummy XML configuration edited.
# virsh domstate --reason dummy shut off (unknown)
# virsh start dummy Domain dummy started
This is a logical next step for us to clean and separate parsing and starting, getting rid of some old code without sacrifying compatibility and maybe generating parser code in the future.
v2: - rebased on top of current master (with virdomainobjlist.c) - only disallow starting domains with invalid definitions (change done in v1), but allow re-defining them - add support for "virsh list --reason" to better excercise the feature added in this patchset
v1: - rebase - don't allow starting domains with invalid state - https://www.redhat.com/archives/libvir-list/2015-November/msg01127.html
RFC: https://www.redhat.com/archives/libvir-list/2015-September/msg00698.html
Martin Kletzander (10): conf, virsh: Add new domain shutoff reason virsh: Refactor the table output of the list command virsh: Add support for list -reason qemu: Few whitespace cleanups conf: Extract name-parsing into its own function conf: Extract UUID parsing into its own function conf: Optionally keep domains with invalid XML, but don't allow starting them qemu: Don't lookup invalid domains unless specified otherwise qemu: Prepare basic APIs to handle invalid defs qemu: Load domains with invalid XML on start
include/libvirt/libvirt-domain.h | 2 + src/bhyve/bhyve_driver.c | 2 + src/conf/domain_conf.c | 121 +++++++++++++++++++++++++++++++-------- 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 | 64 +++++++++++++++++---- src/uml/uml_driver.c | 2 + tests/virshtest.c | 30 +++++++--- tools/virsh-domain-monitor.c | 60 ++++++++++++------- tools/virsh.pod | 5 +- 14 files changed, 303 insertions(+), 69 deletions(-)
I am glad to see these patches you told me before.
And i have test your patches and found some problem until now:
1. if guest xml have private info, libvirt will output it if xml is invalid:
# virsh list --all --reason Id Name State Reason ------------------------------------------------------------
- test3 shut off invalid XML
# virsh -r dumpxml test3 ... <input type='keyboard' bus='ps3'/> <----wrong place
It should be in the same place as it was saved on the disk.
<graphics type='vnc' port='-1' autoport='yes' listen='127.0.0.1' passwd='123'> <----show passwd
Oh, that's definitely what's not suppose to happen, but since we cannot parse it, I will just disable the dumpxml for read-only connections.
2. vcpucount command output looks strange (small problem :) ):
# virsh vcpucount test3
Oh, so my guess was that this happens because virsh calls dumpxml, then searches for the info. But I checked and it calls an API that should be blocked. I'm guessing that happened because of the uuid parsing has gone wrong (below). Anyway, I mentioned the first idea because that can happen with something else and hence the only way how to properly prevent that, is to use different API for invalid domains. I'll try working on that, but this is starting to be bigger and bigger overkill, unfortunately. Anyway, thanks a lot for testing that and letting me know.
3. hit crash if the guest xml uuid is invalid (delete a number) during stop daemon:
Program terminated with signal 11, Segmentation fault. #0 0x00007fcbe773af87 in virDomainDefFree (def=0x7fcbc423d980) at conf/domain_conf.c:2327 2327 virDomainGraphicsDefFree(def->graphics[i]);
Thread 1 (Thread 0x7fcbe835c8c0 (LWP 21589)): #0 0x00007fcbe773af87 in virDomainDefFree (def=0x7fcbc423d980) at conf/domain_conf.c:2327 #1 0x00007fcbe773b903 in virDomainObjDispose (obj=0x7fcbc4235fe0) at conf/domain_conf.c:2487 #2 0x00007fcbe76f182b in virObjectUnref (anyobj=<optimized out>) at util/virobject.c:265 #3 0x00007fcbe76d0ac9 in virHashFree (table=0x7fcbc412eda0) at util/virhash.c:318 #4 0x00007fcbe76f182b in virObjectUnref (anyobj=<optimized out>) at util/virobject.c:265 #5 0x00007fcbce85f8cf in qemuStateCleanup () at qemu/qemu_driver.c:1126 #6 0x00007fcbe779a168 in virStateCleanup () at libvirt.c:815 #7 0x00007fcbe83f02b1 in main (argc=<optimized out>, argv=<optimized out>) at libvirtd.c:1619
Thanks, Luyao
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 12/02/2015 05:13 PM, Martin Kletzander wrote:
On Wed, Dec 02, 2015 at 04:46:20PM +0800, lhuang wrote:
On 12/02/2015 01:35 AM, Martin Kletzander wrote:
We always had to deal with new parsing errors in a weird way. All of them needed to go into functions starting the domains. That messes up the code, it's confusing to newcomers and so on.
I once had an idea that we can handle this stuff. We know what failed, we have the XML that failed parsing and if the problem is not in the domain name nor UUID, we can create a domain object out of that as well. This way we are able to do something we weren't with this series applied. Example follows.
Assume "dummy" is a domain with invalid XML (I just modified the file). Now, just for the purpose of this silly example, let's say we allowed domains with archtecture x86_*, but now we've realized that x86_64 is the only one we want to allow, but there already is a domain defined that has <type arch='x86_256' .../>. With the current master, the domain would be lost, so we would need to modify the funstion starting the domain (e.g. qemuProcessStart for qemu domain). However, with this series, it behaves like this:
# virsh list --all --reason Id Name State Reason --------------------------------------------------------------- - dummy shut off invalid XML
# virsh domstate --reason dummy shut off (invalid XML)
# virsh start dummy error: Failed to start domain dummy error: XML error: domain 'dummy' was not loaded due to an XML error (unsupported configuration: Unknown architecture x86_256), please redefine it
# VISUAL='sed -i s/x86_256/x86_64/' virsh edit dummy Domain dummy XML configuration edited.
# virsh domstate --reason dummy shut off (unknown)
# virsh start dummy Domain dummy started
This is a logical next step for us to clean and separate parsing and starting, getting rid of some old code without sacrifying compatibility and maybe generating parser code in the future.
v2: - rebased on top of current master (with virdomainobjlist.c) - only disallow starting domains with invalid definitions (change done in v1), but allow re-defining them - add support for "virsh list --reason" to better excercise the feature added in this patchset
v1: - rebase - don't allow starting domains with invalid state - https://www.redhat.com/archives/libvir-list/2015-November/msg01127.html
RFC: https://www.redhat.com/archives/libvir-list/2015-September/msg00698.html
Martin Kletzander (10): conf, virsh: Add new domain shutoff reason virsh: Refactor the table output of the list command virsh: Add support for list -reason qemu: Few whitespace cleanups conf: Extract name-parsing into its own function conf: Extract UUID parsing into its own function conf: Optionally keep domains with invalid XML, but don't allow starting them qemu: Don't lookup invalid domains unless specified otherwise qemu: Prepare basic APIs to handle invalid defs qemu: Load domains with invalid XML on start
include/libvirt/libvirt-domain.h | 2 + src/bhyve/bhyve_driver.c | 2 + src/conf/domain_conf.c | 121 +++++++++++++++++++++++++++++++-------- 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 | 64 +++++++++++++++++---- src/uml/uml_driver.c | 2 + tests/virshtest.c | 30 +++++++--- tools/virsh-domain-monitor.c | 60 ++++++++++++------- tools/virsh.pod | 5 +- 14 files changed, 303 insertions(+), 69 deletions(-)
I am glad to see these patches you told me before.
And i have test your patches and found some problem until now:
1. if guest xml have private info, libvirt will output it if xml is invalid:
# virsh list --all --reason Id Name State Reason ------------------------------------------------------------
- test3 shut off invalid XML
# virsh -r dumpxml test3 ... <input type='keyboard' bus='ps3'/> <----wrong place
It should be in the same place as it was saved on the disk.
oh, seems you misunderstood, i mean there is the invalid place which make xml invalid :)
<graphics type='vnc' port='-1' autoport='yes' listen='127.0.0.1' passwd='123'> <----show passwd
Oh, that's definitely what's not suppose to happen, but since we cannot parse it, I will just disable the dumpxml for read-only connections.
okay, sounds good.
2. vcpucount command output looks strange (small problem :) ):
# virsh vcpucount test3
Oh, so my guess was that this happens because virsh calls dumpxml, then searches for the info. But I checked and it calls an API that should be blocked. I'm guessing that happened because of the uuid parsing has gone wrong (below). Anyway, I mentioned the first idea because that can happen with something else and hence the only way how to properly prevent that, is to use different API for invalid domains. I'll try working on that, but this is starting to be bigger and bigger overkill, unfortunately.
I checked the code and found virshCPUCountCollect will return -1: if (checkState && ((flags & VIR_DOMAIN_AFFECT_LIVE && virDomainIsActive(dom) < 1) || (flags & VIR_DOMAIN_AFFECT_CONFIG && virDomainIsPersistent(dom) < 1))) return -1; and PRINT_COUNT will skip which var <= 0, so command return success and no output.
Anyway, thanks a lot for testing that and letting me know.
You're welcome, i will reply this mail if i can find more problem. Luyao
3. hit crash if the guest xml uuid is invalid (delete a number) during stop daemon:
Program terminated with signal 11, Segmentation fault. #0 0x00007fcbe773af87 in virDomainDefFree (def=0x7fcbc423d980) at conf/domain_conf.c:2327 2327 virDomainGraphicsDefFree(def->graphics[i]);
Thread 1 (Thread 0x7fcbe835c8c0 (LWP 21589)): #0 0x00007fcbe773af87 in virDomainDefFree (def=0x7fcbc423d980) at conf/domain_conf.c:2327 #1 0x00007fcbe773b903 in virDomainObjDispose (obj=0x7fcbc4235fe0) at conf/domain_conf.c:2487 #2 0x00007fcbe76f182b in virObjectUnref (anyobj=<optimized out>) at util/virobject.c:265 #3 0x00007fcbe76d0ac9 in virHashFree (table=0x7fcbc412eda0) at util/virhash.c:318 #4 0x00007fcbe76f182b in virObjectUnref (anyobj=<optimized out>) at util/virobject.c:265 #5 0x00007fcbce85f8cf in qemuStateCleanup () at qemu/qemu_driver.c:1126 #6 0x00007fcbe779a168 in virStateCleanup () at libvirt.c:815 #7 0x00007fcbe83f02b1 in main (argc=<optimized out>, argv=<optimized out>) at libvirtd.c:1619
Thanks, Luyao
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 12/01/2015 12:35 PM, Martin Kletzander wrote:
We always had to deal with new parsing errors in a weird way. All of them needed to go into functions starting the domains. That messes up the code, it's confusing to newcomers and so on.
I once had an idea that we can handle this stuff. We know what failed, we have the XML that failed parsing and if the problem is not in the domain name nor UUID, we can create a domain object out of that as well. This way we are able to do something we weren't with this series applied. Example follows.
Assume "dummy" is a domain with invalid XML (I just modified the file). Now, just for the purpose of this silly example, let's say we allowed domains with archtecture x86_*, but now we've realized that x86_64 is the only one we want to allow, but there already is a domain defined that has <type arch='x86_256' .../>. With the current master, the domain would be lost, so we would need to modify the funstion starting the domain (e.g. qemuProcessStart for qemu domain). However, with this series, it behaves like this:
# virsh list --all --reason Id Name State Reason --------------------------------------------------------------- - dummy shut off invalid XML
# virsh domstate --reason dummy shut off (invalid XML)
# virsh start dummy error: Failed to start domain dummy error: XML error: domain 'dummy' was not loaded due to an XML error (unsupported configuration: Unknown architecture x86_256), please redefine it
# VISUAL='sed -i s/x86_256/x86_64/' virsh edit dummy Domain dummy XML configuration edited.
# virsh domstate --reason dummy shut off (unknown)
# virsh start dummy Domain dummy started
This is a logical next step for us to clean and separate parsing and starting, getting rid of some old code without sacrifying compatibility and maybe generating parser code in the future.
v2: - rebased on top of current master (with virdomainobjlist.c) - only disallow starting domains with invalid definitions (change done in v1), but allow re-defining them - add support for "virsh list --reason" to better excercise the feature added in this patchset
v1: - rebase - don't allow starting domains with invalid state - https://www.redhat.com/archives/libvir-list/2015-November/msg01127.html
RFC: https://www.redhat.com/archives/libvir-list/2015-September/msg00698.html
Martin Kletzander (10): conf, virsh: Add new domain shutoff reason virsh: Refactor the table output of the list command virsh: Add support for list -reason qemu: Few whitespace cleanups conf: Extract name-parsing into its own function conf: Extract UUID parsing into its own function conf: Optionally keep domains with invalid XML, but don't allow starting them qemu: Don't lookup invalid domains unless specified otherwise qemu: Prepare basic APIs to handle invalid defs qemu: Load domains with invalid XML on start
include/libvirt/libvirt-domain.h | 2 + src/bhyve/bhyve_driver.c | 2 + src/conf/domain_conf.c | 121 +++++++++++++++++++++++++++++++-------- 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 | 64 +++++++++++++++++---- src/uml/uml_driver.c | 2 + tests/virshtest.c | 30 +++++++--- tools/virsh-domain-monitor.c | 60 ++++++++++++------- tools/virsh.pod | 5 +- 14 files changed, 303 insertions(+), 69 deletions(-)
-- 2.6.3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Beyond the few noted spots changes look good to me. Implicit ACK for those not specifically noted. John
On Thu, Dec 10, 2015 at 03:20:20PM -0500, John Ferlan wrote:
On 12/01/2015 12:35 PM, Martin Kletzander wrote:
We always had to deal with new parsing errors in a weird way. All of them needed to go into functions starting the domains. That messes up the code, it's confusing to newcomers and so on.
I once had an idea that we can handle this stuff. We know what failed, we have the XML that failed parsing and if the problem is not in the domain name nor UUID, we can create a domain object out of that as well. This way we are able to do something we weren't with this series applied. Example follows.
Assume "dummy" is a domain with invalid XML (I just modified the file). Now, just for the purpose of this silly example, let's say we allowed domains with archtecture x86_*, but now we've realized that x86_64 is the only one we want to allow, but there already is a domain defined that has <type arch='x86_256' .../>. With the current master, the domain would be lost, so we would need to modify the funstion starting the domain (e.g. qemuProcessStart for qemu domain). However, with this series, it behaves like this:
# virsh list --all --reason Id Name State Reason --------------------------------------------------------------- - dummy shut off invalid XML
# virsh domstate --reason dummy shut off (invalid XML)
# virsh start dummy error: Failed to start domain dummy error: XML error: domain 'dummy' was not loaded due to an XML error (unsupported configuration: Unknown architecture x86_256), please redefine it
# VISUAL='sed -i s/x86_256/x86_64/' virsh edit dummy Domain dummy XML configuration edited.
# virsh domstate --reason dummy shut off (unknown)
# virsh start dummy Domain dummy started
This is a logical next step for us to clean and separate parsing and starting, getting rid of some old code without sacrifying compatibility and maybe generating parser code in the future.
v2: - rebased on top of current master (with virdomainobjlist.c) - only disallow starting domains with invalid definitions (change done in v1), but allow re-defining them - add support for "virsh list --reason" to better excercise the feature added in this patchset
v1: - rebase - don't allow starting domains with invalid state - https://www.redhat.com/archives/libvir-list/2015-November/msg01127.html
RFC: https://www.redhat.com/archives/libvir-list/2015-September/msg00698.html
Martin Kletzander (10): conf, virsh: Add new domain shutoff reason virsh: Refactor the table output of the list command virsh: Add support for list -reason qemu: Few whitespace cleanups conf: Extract name-parsing into its own function conf: Extract UUID parsing into its own function conf: Optionally keep domains with invalid XML, but don't allow starting them qemu: Don't lookup invalid domains unless specified otherwise qemu: Prepare basic APIs to handle invalid defs qemu: Load domains with invalid XML on start
include/libvirt/libvirt-domain.h | 2 + src/bhyve/bhyve_driver.c | 2 + src/conf/domain_conf.c | 121 +++++++++++++++++++++++++++++++-------- 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 | 64 +++++++++++++++++---- src/uml/uml_driver.c | 2 + tests/virshtest.c | 30 +++++++--- tools/virsh-domain-monitor.c | 60 ++++++++++++------- tools/virsh.pod | 5 +- 14 files changed, 303 insertions(+), 69 deletions(-)
-- 2.6.3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Beyond the few noted spots changes look good to me. Implicit ACK for those not specifically noted.
Thanks a lot, but there is still the security issue and a crash mentioned by Luyao. I know how to deal with only a part of it. Anyway, this will need another version, so I'll include all the nits pointed out in there, but it'll take some time again because from my POV this is just a nice-to-have feature and nobody is asking for this, so there are other, more pressing, things in the priority queue and hence this one will have to wait again. Anyway, thanks again for checking this out.
John
[...]
Beyond the few noted spots changes look good to me. Implicit ACK for those not specifically noted.
Thanks a lot, but there is still the security issue and a crash mentioned by Luyao. I know how to deal with only a part of it. Anyway, this will need another version, so I'll include all the nits pointed out in there, but it'll take some time again because from my POV this is just a nice-to-have feature and nobody is asking for this, so there are other, more pressing, things in the priority queue and hence this one will have to wait again. Anyway, thanks again for checking this out.
right understood - although if you wanted to make at least some progress - it seems patches 2-5 are separable. Patch 2&3 adds a "nice to have" --reason output for the virsh table which is useful without the invalid XML output... Although, as I thought about 2-3 a bit more, I have to think there are other tests out there (like virttest/avacado) which compare output of commands to stock/previous output and complain about failures when something is slightly different. I recall having to fix up a bunch when the output changed to add a single space before the header and data rows... E.G from: Id ... --- ... 1 ... to Id ... --- ... 1 ... So those 3 missing '-' could cause agita for someone. John
participants (3)
-
John Ferlan -
lhuang -
Martin Kletzander