[libvirt] [PATCH 0/4] Add ability to store and display notes and description with domains
This patchset adds ability to store, display and modify comments for domains to help administrators identify and store metadata to domains to allow easy identification. A short description "note" was added as an attribute for the <description> element to hold a shorter description (limited to max 40 characters) to be shown along with lists of domains. A new command is being added to virsh to accomodate work with these new options. Peter Krempa (4): xml: Add attribute to <description> element to hold a short note api: Add api to set domain note and description in runtime qemu: Implement note API in qemu driver virsh: Add support for modifying domain description and notes docs/formatdomain.html.in | 7 +- docs/schemas/domaincommon.rng | 14 ++- include/libvirt/libvirt.h.in | 15 +++ src/conf/domain_conf.c | 27 ++++- src/conf/domain_conf.h | 1 + src/driver.h | 5 + src/libvirt.c | 47 ++++++++ src/libvirt_public.syms | 5 + src/qemu/qemu_driver.c | 70 ++++++++++++ src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 9 ++- tools/virsh.c | 246 +++++++++++++++++++++++++++++++++++++---- tools/virsh.pod | 30 +++++- 13 files changed, 450 insertions(+), 27 deletions(-) -- 1.7.3.4
This patch adds an optional attribute note="" to the <description> element in the domain XML. This attribute can hold a short note defined by the user to ease the identification of domains. The note is limited to 40 characters. *docs/formatdomain.html.in *docs/schemas/domaincommon.rng - add schema grammar for the new element and documentation *src/conf/domain_conf.c *src/conf/domain_conf.h - add field to hold the new attribute - add code to parse and create XML with the new attribute --- docs/formatdomain.html.in | 7 +++++-- docs/schemas/domaincommon.rng | 14 +++++++++++++- src/conf/domain_conf.c | 27 +++++++++++++++++++++++++-- src/conf/domain_conf.h | 1 + 4 files changed, 44 insertions(+), 5 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 4b31e2a..0315734 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -32,7 +32,8 @@ <domain type='xen' id='3'> <name>fv0</name> <uuid>4dea22b31d52d8f32516782e98ab3fa0</uuid> - <description>Some human readable description</description> + <description note='Some short description'> + Some human readable (optional) long description</description> ...</pre> <dl> @@ -58,7 +59,9 @@ <dd>The content of the <code>description</code> element provides a human readable description of the virtual machine. This data is not used by libvirt in any way, it can contain any information the user - wants. <span class="since">Since 0.7.2</span></dd> + wants. <span class="since">Since 0.7.2</span> The optional attribute + <code>note</code> provides space for a shorter description. + <span class="since">Since 0.9.10</span></dd> </dl> <h3><a name="elementsOS">Operating system booting</a></h3> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index fdcbc28..0cf5caf 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -10,7 +10,19 @@ --> <define name="description"> <element name="description"> - <text/> + <optional> + <attribute name="note"> + <data type="string"/> + </attribute> + </optional> + <choice> + <group> + <text/> + </group> + <group> + <empty/> + </group> + </choice> </element> </define> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d474551..d1e0ab2 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1472,6 +1472,7 @@ void virDomainDefFree(virDomainDefPtr def) VIR_FREE(def->cpumask); VIR_FREE(def->emulator); VIR_FREE(def->description); + VIR_FREE(def->note); virBlkioDeviceWeightArrayClear(def->blkio.devices, def->blkio.ndevices); @@ -7103,6 +7104,22 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, VIR_FREE(tmp); } + /* Extract short description of domain (note) */ + def->note = virXPathString("string(./description[1]/@note)", ctxt); + if (def->note) { + if (strchr(def->note, '\n')) { + virDomainReportError(VIR_ERR_XML_ERROR, + "%s", _("Note attribute can't contain newlines")); + goto error; + } + + if (strlen(def->note) > 40) { + virDomainReportError(VIR_ERR_XML_ERROR, + "%s", _("Note too long")); + goto error; + } + } + /* Extract documentation if present */ def->description = virXPathString("string(./description[1])", ctxt); @@ -11422,8 +11439,14 @@ virDomainDefFormatInternal(virDomainDefPtr def, virUUIDFormat(uuid, uuidstr); virBufferAsprintf(buf, " <uuid>%s</uuid>\n", uuidstr); - virBufferEscapeString(buf, " <description>%s</description>\n", - def->description); + if (def->note || def->description) { + virBufferAddLit(buf, " <description"); + virBufferEscapeString(buf, " note='%s'", def->note); + if (def->description) + virBufferEscapeString(buf, ">%s</description>\n", def->description); + else + virBufferAddLit(buf, "/>\n"); + } virBufferAsprintf(buf, " <memory>%lu</memory>\n", def->mem.max_balloon); virBufferAsprintf(buf, " <currentMemory>%lu</currentMemory>\n", diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index bac5a87..208a011 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1409,6 +1409,7 @@ struct _virDomainDef { int id; unsigned char uuid[VIR_UUID_BUFLEN]; char *name; + char *note; char *description; struct { -- 1.7.3.4
On Fri, Jan 13, 2012 at 07:17:36PM +0100, Peter Krempa wrote:
This patch adds an optional attribute note="" to the <description> element in the domain XML. This attribute can hold a short note defined by the user to ease the identification of domains. The note is limited to 40 characters.
I think it would be better to use a new element for this rather than an attribute. eg <title>Foo bar wizz</title> Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 01/13/2012 11:17 AM, Peter Krempa wrote:
This patch adds an optional attribute note="" to the <description> element in the domain XML. This attribute can hold a short note defined by the user to ease the identification of domains. The note is limited to 40 characters.
I'd go one step further, and reject newlines.
*docs/formatdomain.html.in *docs/schemas/domaincommon.rng - add schema grammar for the new element and documentation
*src/conf/domain_conf.c *src/conf/domain_conf.h - add field to hold the new attribute - add code to parse and create XML with the new attribute --- docs/formatdomain.html.in | 7 +++++-- docs/schemas/domaincommon.rng | 14 +++++++++++++- src/conf/domain_conf.c | 27 +++++++++++++++++++++++++-- src/conf/domain_conf.h | 1 + 4 files changed, 44 insertions(+), 5 deletions(-)
@@ -58,7 +59,9 @@ <dd>The content of the <code>description</code> element provides a human readable description of the virtual machine. This data is not used by libvirt in any way, it can contain any information the user - wants. <span class="since">Since 0.7.2</span></dd> + wants. <span class="since">Since 0.7.2</span> The optional attribute + <code>note</code> provides space for a shorter description. + <span class="since">Since 0.9.10</span></dd>
The end result has confusing punctuation. I'd suggest: + wants, <span class="since">since 0.7.2</span>. The optional attribute + <code>note</code> provides space for a shorter description, + capped at 40 bytes and with no newline, + <span class="since">since 0.9.10</span>.</dd>
+++ b/docs/schemas/domaincommon.rng @@ -10,7 +10,19 @@ --> <define name="description"> <element name="description"> - <text/> + <optional> + <attribute name="note"> + <data type="string"/>
We can make this enforce length restrictions: <data type="string"> <param name='pattern'>[^\n]{0,40}</param> </data>
+ </attribute> + </optional> + <choice> + <group> + <text/> + </group> + <group> + <empty/> + </group> + </choice>
No need for a <group> around a single element. This whole layout may change if we go with an optional <title> instead of <description note='...'>, but I'd almost rather see: <element name='description'> <choice> <group> <attribute name='note'>...</attribute> <choice> <text/> <empty/> </choice> </group> <text/> </choice> </element> that is, you must have the attribute, or text, or both, but not <description/> with neither.
+ /* Extract short description of domain (note) */ + def->note = virXPathString("string(./description[1]/@note)", ctxt); + if (def->note) { + if (strchr(def->note, '\n')) { + virDomainReportError(VIR_ERR_XML_ERROR, + "%s", _("Note attribute can't contain newlines")); + goto error; + } + + if (strlen(def->note) > 40) {
This checks for bytes, not characters (some 40-character UTF-8 strings can occupy well more than 100 bytes). I'm probably okay with a byte limit, but hope we don't get complaints about someone unable to write a single word of 12 characters in their favorite locale. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
The <description> element was accessible only throught the XML, that makes it hard to work with. Atomic operations and modification of the live domain are impossible. This patch adds a function to the public API to set arbitrary descriptions for live and persistent domains. *include/libvirt/libvirt.h.in - add api function virDomainSetDescription - add flags for this new api *src/driver.h *src/libvirt.c - add driver support - implement the public api *src/libvirt_public.syms - export the new function *src/remote/remote_driver.c *src/remote/remote_protocol.x - wire up the remote protocol --- include/libvirt/libvirt.h.in | 15 +++++++++++++ src/driver.h | 5 ++++ src/libvirt.c | 47 ++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 ++++ src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 9 +++++++- 6 files changed, 81 insertions(+), 1 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index e436f3c..7edbb9c 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1405,6 +1405,21 @@ int virDomainGetMaxVcpus (virDomainPtr domain); int virDomainGetSecurityLabel (virDomainPtr domain, virSecurityLabelPtr seclabel); +typedef enum { + /* See virDomainModificationImpact for these flags. */ + VIR_DOMAIN_DESCRIPTION_CURRENT = VIR_DOMAIN_AFFECT_CURRENT, + VIR_DOMAIN_DESCRIPTION_LIVE = VIR_DOMAIN_AFFECT_LIVE, + VIR_DOMAIN_DESCRIPTION_CONFIG = VIR_DOMAIN_AFFECT_CONFIG, + /* Additionaly, these flags may be bitwise-OR'd in. */ + VIR_DOMAIN_DESCRIPTION_NOTE = (1 << 2), /* Operate on note instead of + the complete description + field */ +} virDomainDescriptionFlags; + +int virDomainSetDescription(virDomainPtr domain, + const char *description, + unsigned int flags); + /* * XML domain description */ diff --git a/src/driver.h b/src/driver.h index 24636a4..afb8f3c 100644 --- a/src/driver.h +++ b/src/driver.h @@ -793,6 +793,10 @@ typedef int virTypedParameterPtr params, int *nparams, unsigned int flags); +typedef int + (*virDrvDomainSetDescription)(virDomainPtr dom, + const char *description, + unsigned int flags); /** * _virDriver: @@ -962,6 +966,7 @@ struct _virDriver { virDrvNodeSuspendForDuration nodeSuspendForDuration; virDrvDomainSetBlockIoTune domainSetBlockIoTune; virDrvDomainGetBlockIoTune domainGetBlockIoTune; + virDrvDomainSetDescription domainSetDescription; }; typedef int diff --git a/src/libvirt.c b/src/libvirt.c index a540424..c0f052b 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -8746,6 +8746,53 @@ error: } /** + * virDomainSetDescription: + * @domain: a domain object + * @description: new description text + * @flags: bitwise-OR of virDomainDescriptionFlags + * + * Sets the domain description field or note field depending on the flags + * parameter. + * + * Returns 0 on success, -1 in case of failure; + */ +int +virDomainSetDescription(virDomainPtr domain, + const char *description, + unsigned int flags) +{ + virConnectPtr conn; + + VIR_DOMAIN_DEBUG(domain, "description=%p, flags=%x", description, flags); + + if (!VIR_IS_CONNECTED_DOMAIN(domain)) { + virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); + goto error; + } + + if (description == NULL) { + virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); + goto error; + } + + conn = domain->conn; + + if (conn->driver->domainSetDescription) { + int ret; + ret = conn->driver->domainSetDescription(domain, description, flags); + if (ret < 0) + goto error; + return ret; + } + + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(domain->conn); + return -1; +} + +/** * virNodeGetSecurityModel: * @conn: a connection object * @secmodel: pointer to a virSecurityModel structure diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 4ca7216..882b746 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -516,4 +516,9 @@ LIBVIRT_0.9.9 { virDomainSetNumaParameters; } LIBVIRT_0.9.8; +LIBVIRT_0.9.10 { + global: + virDomainSetDescription; +} LIBVIRT_0.9.9; + # .... define new API here using predicted next version number .... diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index e28840b..4d64bc9 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -4750,6 +4750,7 @@ static virDriver remote_driver = { .domainGetBlockIoTune = remoteDomainGetBlockIoTune, /* 0.9.8 */ .domainSetNumaParameters = remoteDomainSetNumaParameters, /* 0.9.9 */ .domainGetNumaParameters = remoteDomainGetNumaParameters, /* 0.9.9 */ + .domainSetDescription = remoteDomainSetDescription, /* 0.9.10 */ }; static virNetworkDriver network_driver = { diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index ca739ff..ad3e12f 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -1095,6 +1095,12 @@ struct remote_domain_set_autostart_args { int autostart; }; +struct remote_domain_set_description_args { + remote_nonnull_domain dom; + remote_nonnull_string description; + unsigned int flags; +}; + struct remote_domain_block_job_abort_args { remote_nonnull_domain dom; remote_nonnull_string path; @@ -2653,7 +2659,8 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_SET_NUMA_PARAMETERS = 254, /* autogen autogen */ REMOTE_PROC_DOMAIN_GET_NUMA_PARAMETERS = 255, /* skipgen skipgen */ REMOTE_PROC_DOMAIN_SET_INTERFACE_PARAMETERS = 256, /* autogen autogen */ - REMOTE_PROC_DOMAIN_GET_INTERFACE_PARAMETERS = 257 /* skipgen skipgen */ + REMOTE_PROC_DOMAIN_GET_INTERFACE_PARAMETERS = 257, /* skipgen skipgen */ + REMOTE_PROC_DOMAIN_SET_DESCRIPTION = 258 /* autogen autogen */ /* * Notice how the entries are grouped in sets of 10 ? -- 1.7.3.4
On 01/13/2012 11:17 AM, Peter Krempa wrote:
The <description> element was accessible only throught the XML, that
s/throught/through/
makes it hard to work with. Atomic operations and modification of the live domain are impossible. This patch adds a function to the public API to set arbitrary descriptions for live and persistent domains.
Do we want a counterpart getter function, or is dumpxml and XPath filtering good enough?
+++ b/include/libvirt/libvirt.h.in @@ -1405,6 +1405,21 @@ int virDomainGetMaxVcpus (virDomainPtr domain); int virDomainGetSecurityLabel (virDomainPtr domain, virSecurityLabelPtr seclabel);
+typedef enum { + /* See virDomainModificationImpact for these flags. */ + VIR_DOMAIN_DESCRIPTION_CURRENT = VIR_DOMAIN_AFFECT_CURRENT, + VIR_DOMAIN_DESCRIPTION_LIVE = VIR_DOMAIN_AFFECT_LIVE, + VIR_DOMAIN_DESCRIPTION_CONFIG = VIR_DOMAIN_AFFECT_CONFIG,
I'm not sure whether we need to create these new aliases, or just state that @flags is a mix of virDomainModificationImpact and virDomainDescriptionFlags, and leave a comment here stating why bits 0 and 1 are reserved.
+ /* Additionaly, these flags may be bitwise-OR'd in. */
s/Additionaly/Additionally/
+++ b/src/libvirt.c @@ -8746,6 +8746,53 @@ error: }
/** + * virDomainSetDescription: + * @domain: a domain object + * @description: new description text + * @flags: bitwise-OR of virDomainDescriptionFlags + * + * Sets the domain description field or note field depending on the flags + * parameter. + * + * Returns 0 on success, -1 in case of failure;
Looks reasonable.
+ */ +int +virDomainSetDescription(virDomainPtr domain, + const char *description, + unsigned int flags) +{ + virConnectPtr conn; + + VIR_DOMAIN_DEBUG(domain, "description=%p, flags=%x", description, flags); + + if (!VIR_IS_CONNECTED_DOMAIN(domain)) { + virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); + goto error; + }
You need to reject this operation on a read-only connection. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
This patch adds support for the new api to change domain descriptions to the qemu driver. --- src/qemu/qemu_driver.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 70 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 712f1fc..7b935f0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11831,6 +11831,75 @@ cleanup: return ret; } +static int +qemuDomainSetDescription(virDomainPtr dom, const char *description, + unsigned int flags) +{ + struct qemud_driver *driver = dom->conn->privateData; + virDomainObjPtr vm; + virDomainDefPtr persistentDef; + int ret = -1; + + virCheckFlags(VIR_DOMAIN_DESCRIPTION_CURRENT | + VIR_DOMAIN_DESCRIPTION_LIVE | + VIR_DOMAIN_DESCRIPTION_CONFIG | + VIR_DOMAIN_DESCRIPTION_NOTE, -1); + + bool note = (flags | VIR_DOMAIN_DESCRIPTION_NOTE) > 0; + + qemuDriverLock(driver); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + qemuDriverUnlock(driver); + + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + qemuReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); + goto cleanup; + } + + if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, + &persistentDef) < 0) + goto cleanup; + + if (flags | VIR_DOMAIN_DESCRIPTION_LIVE) { + if (note) { + VIR_FREE(vm->def->note); + if (!(vm->def->note = strdup(description))) + goto oom; + } else { + VIR_FREE(vm->def->description); + if (!(vm->def->description = strdup(description))) + goto oom; + } + } + + if (flags | VIR_DOMAIN_DESCRIPTION_CONFIG) { + if (note) { + VIR_FREE(persistentDef->note); + if (!(persistentDef->note = strdup(description))) + goto oom; + } else { + VIR_FREE(persistentDef->description); + if (!(persistentDef->description = strdup(description))) + goto oom; + } + } + + ret = 0; + +cleanup: + if (vm) + virDomainObjUnlock(vm); + return ret; +oom: + if (vm) + virDomainObjUnlock(vm); + virReportOOMError(); + return -1; +} + static virDriver qemuDriver = { .no = VIR_DRV_QEMU, .name = "QEMU", @@ -11983,6 +12052,7 @@ static virDriver qemuDriver = { .domainGetNumaParameters = qemuDomainGetNumaParameters, /* 0.9.9 */ .domainGetInterfaceParameters = qemuDomainGetInterfaceParameters, /* 0.9.9 */ .domainSetInterfaceParameters = qemuDomainSetInterfaceParameters, /* 0.9.9 */ + .domainSetDescription = qemuDomainSetDescription, /* 0.9.10 */ }; -- 1.7.3.4
On 01/13/2012 11:17 AM, Peter Krempa wrote:
This patch adds support for the new api to change domain descriptions to the qemu driver. --- src/qemu/qemu_driver.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 70 insertions(+), 0 deletions(-)
This seems simple enough that you should support it for more drivers; at least test and lxc are good candidates.
+static int +qemuDomainSetDescription(virDomainPtr dom, const char *description, + unsigned int flags) +{ + struct qemud_driver *driver = dom->conn->privateData; + virDomainObjPtr vm; + virDomainDefPtr persistentDef; + int ret = -1; + + virCheckFlags(VIR_DOMAIN_DESCRIPTION_CURRENT |
See my comment in 2/4 about just re-using VIR_DOMAIN_AFFECT_CURRENT rather than adding a new alias. Also, this particular enum value is 0, so you can omit it from this virCheckFlags list.
+ VIR_DOMAIN_DESCRIPTION_LIVE | + VIR_DOMAIN_DESCRIPTION_CONFIG | + VIR_DOMAIN_DESCRIPTION_NOTE, -1); + + bool note = (flags | VIR_DOMAIN_DESCRIPTION_NOTE) > 0;
Wrong operator - you want &, not |.
+ + if (flags | VIR_DOMAIN_DESCRIPTION_LIVE) {
And again.
+ + if (flags | VIR_DOMAIN_DESCRIPTION_CONFIG) {
And again.
+ if (note) { + VIR_FREE(persistentDef->note); + if (!(persistentDef->note = strdup(description))) + goto oom; + } else { + VIR_FREE(persistentDef->description); + if (!(persistentDef->description = strdup(description))) + goto oom; + } + }
Missing a call to flush the new definition to disk - basically, you need to call virDomainSaveConfig somewhere in this method. To test whether things worked, use virsh to alter the description, then restart libvirtd, then dumpxml to see if the alterations stuck around.
+ + ret = 0; + +cleanup: + if (vm) + virDomainObjUnlock(vm); + return ret; +oom:
Hmm - elsewhere in qemu_driver.c, we call this sort of label no_memory or out_of_memory. Also, I'd consider making it simpler, by not duplicating the rest of cleanup: oom: virReportOOMError(); goto cleanup; -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
This patch adds a new command "desc" to show and modify notes and description for the domains using the new API. This patch also adds a new flag for the "list" command to show notes in the domain list, to allow easy identification of VMs by storing a short description. Example: virsh # list --note Id Name State Note ----------------------------------------------- 0 Domain-0 running Mailserver 1 2 fedora paused --- tools/virsh.c | 246 ++++++++++++++++++++++++++++++++++++++++++++++++++----- tools/virsh.pod | 30 +++++++- 2 files changed, 255 insertions(+), 21 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index f223593..a1f9236 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -312,6 +312,9 @@ static int vshCommandOptULongLong(const vshCmd *cmd, const char *name, static bool vshCommandOptBool(const vshCmd *cmd, const char *name); static const vshCmdOpt *vshCommandOptArgv(const vshCmd *cmd, const vshCmdOpt *opt); +static char *vshGetDomainDescription(vshControl *ctl, virDomainPtr dom, + bool note, unsigned int flags) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; #define VSH_BYID (1 << 1) #define VSH_BYUUID (1 << 2) @@ -885,6 +888,7 @@ static const vshCmdOptDef opts_list[] = { {"all", VSH_OT_BOOL, 0, N_("list inactive & active domains")}, {"managed-save", VSH_OT_BOOL, 0, N_("mark domains with managed save state")}, + {"note", VSH_OT_BOOL, 0, N_("show short domain description")}, {NULL, 0, 0, NULL} }; @@ -899,7 +903,10 @@ cmdList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) char **names = NULL; int maxname = 0; bool managed = vshCommandOptBool(cmd, "managed-save"); + bool desc = vshCommandOptBool(cmd, "note"); + char *note; int state; + bool ret = false; inactive |= all; @@ -917,8 +924,7 @@ cmdList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) if ((maxid = virConnectListDomains(ctl->conn, &ids[0], maxid)) < 0) { vshError(ctl, "%s", _("Failed to list active domains")); - VIR_FREE(ids); - return false; + goto cleanup; } qsort(&ids[0], maxid, sizeof(int), idsorter); @@ -928,37 +934,52 @@ cmdList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) maxname = virConnectNumOfDefinedDomains(ctl->conn); if (maxname < 0) { vshError(ctl, "%s", _("Failed to list inactive domains")); - VIR_FREE(ids); - return false; + goto cleanup; } if (maxname) { names = vshMalloc(ctl, sizeof(char *) * maxname); if ((maxname = virConnectListDefinedDomains(ctl->conn, names, maxname)) < 0) { vshError(ctl, "%s", _("Failed to list inactive domains")); - VIR_FREE(ids); - VIR_FREE(names); - return false; + goto cleanup; } qsort(&names[0], maxname, sizeof(char*), namesorter); } } - vshPrintExtra(ctl, " %-5s %-30s %s\n", _("Id"), _("Name"), _("State")); - vshPrintExtra(ctl, "----------------------------------------------------\n"); + + if (desc) { + vshPrintExtra(ctl, "%-5s %-30s %-10s %s\n", _("Id"), _("Name"), _("State"), _("Note")); + vshPrintExtra(ctl, "-----------------------------------------------------------\n"); + } else { + vshPrintExtra(ctl, " %-5s %-30s %s\n", _("Id"), _("Name"), _("State")); + vshPrintExtra(ctl, "----------------------------------------------------\n"); + } for (i = 0; i < maxid; i++) { - virDomainPtr dom = virDomainLookupByID(ctl->conn, ids[i]); + virDomainPtr dom = virDomainLookupByID(ctl->conn, ids[i]); /* this kind of work with domains is not atomic operation */ if (!dom) continue; - vshPrint(ctl, " %-5d %-30s %s\n", - virDomainGetID(dom), - virDomainGetName(dom), - _(vshDomainStateToString(vshDomainState(ctl, dom, NULL)))); - virDomainFree(dom); + if (desc) { + if (!(note = vshGetDomainDescription(ctl, dom, true, 0))) + goto cleanup; + + vshPrint(ctl, "%-5d %-30s %-10s %s\n", + virDomainGetID(dom), + virDomainGetName(dom), + _(vshDomainStateToString(vshDomainState(ctl, dom, NULL))), + note); + VIR_FREE(note); + } else { + vshPrint(ctl, " %-5d %-30s %s\n", + virDomainGetID(dom), + virDomainGetName(dom), + _(vshDomainStateToString(vshDomainState(ctl, dom, NULL)))); + } + virDomainFree(dom); } for (i = 0; i < maxname; i++) { virDomainPtr dom = virDomainLookupByName(ctl->conn, names[i]); @@ -974,17 +995,163 @@ cmdList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) virDomainHasManagedSaveImage(dom, 0) > 0) state = -2; - vshPrint(ctl, " %-5s %-30s %s\n", - "-", - names[i], - state == -2 ? _("saved") : _(vshDomainStateToString(state))); + if (desc) { + if (!(note = vshGetDomainDescription(ctl, dom, true, 0))) + goto cleanup; + + vshPrint(ctl, "%-5s %-30s %-10s %s\n", + "-", + names[i], + state == -2 ? _("saved") : _(vshDomainStateToString(state)), + note); + VIR_FREE(note); + } else { + vshPrint(ctl, " %-5s %-30s %s\n", + "-", + names[i], + state == -2 ? _("saved") : _(vshDomainStateToString(state))); virDomainFree(dom); VIR_FREE(names[i]); + } } + + ret = true; +cleanup: VIR_FREE(ids); VIR_FREE(names); - return true; + return ret; +} + +/* + * "desc" command for managing domain description and note + */ +static const vshCmdInfo info_desc[] = { + {"help", N_("show or set domain's description or note")}, + {"desc", N_("Allows to show or modify description or note of a domain.")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_desc[] = { + {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, + {"persistent", VSH_OT_BOOL, 0, N_("modify persistent state of domain")}, + {"live", VSH_OT_BOOL, 0, N_("modify note only for current instance")}, + {"note", VSH_OT_BOOL, 0, N_("modify the note instead of description")}, + {"edit", VSH_OT_BOOL, 0, N_("open an editor to modify the description")}, + {"new_desc", VSH_OT_ARGV, 0, N_("message")}, + {NULL, 0, 0, NULL} +}; + +static bool +cmdDesc(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) +{ + virDomainPtr dom; + bool inactive = vshCommandOptBool(cmd, "persistent"); + bool live = vshCommandOptBool(cmd, "live"); + + bool note = vshCommandOptBool(cmd, "note"); + bool edit = vshCommandOptBool(cmd, "edit"); + + int state; + char *desc = NULL; + char *desc_edited = NULL; + char *tmp = NULL; + const vshCmdOpt *opt = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + bool pad = false; + bool ret = false; + unsigned int flags = VIR_DOMAIN_DESCRIPTION_CURRENT; + + if (!vshConnectionUsability(ctl, ctl->conn)) + return false; + + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) + return false; + + if ((state = vshDomainState(ctl, dom, NULL)) < 0) { + ret = false; + goto cleanup; + } + + while ((opt = vshCommandOptArgv(cmd, opt))) { + if (pad) + virBufferAddChar(&buf, ' '); + pad = true; + virBufferAdd(&buf, opt->data, -1); + } + + if (live) + flags |= VIR_DOMAIN_DESCRIPTION_LIVE; + if (inactive) + flags |= VIR_DOMAIN_DESCRIPTION_CONFIG; + if (!(inactive || live)) { + flags |= VIR_DOMAIN_DESCRIPTION_CONFIG; + if (state == VIR_DOMAIN_RUNNING || state == VIR_DOMAIN_PAUSED) + flags |= VIR_DOMAIN_DESCRIPTION_LIVE; + } + + if (virBufferError(&buf)) { + vshPrint(ctl, "%s", _("Failed to collect new description/note")); + goto cleanup; + } + desc = virBufferContentAndReset(&buf); + + if (edit || desc) { + if (!desc) { + desc = vshGetDomainDescription(ctl, dom, note, + inactive?VIR_DOMAIN_XML_INACTIVE:0); + if (!desc) + goto cleanup; + } + + if (edit) { + /* Create and open the temporary file. */ + tmp = editWriteToTempFile (ctl, desc); + if (!tmp) goto cleanup; + + /* Start the editor. */ + if (editFile (ctl, tmp) == -1) goto cleanup; + + /* Read back the edited file. */ + desc_edited = editReadBackFile (ctl, tmp); + if (!desc_edited) goto cleanup; + + /* Compare original XML with edited. Has it changed at all? */ + if (STREQ (desc, desc_edited)) { + vshPrint (ctl, _("Domain description not changed.\n")); + ret = true; + goto cleanup; + } + + VIR_FREE(desc); + desc = desc_edited; + desc_edited = NULL; + } + + if (virDomainSetDescription(dom, desc, flags) < 0) { + vshError(ctl, "%s", + _("Failed to set new domain description")); + goto cleanup; + } + } else { + desc = vshGetDomainDescription(ctl, dom, note, + inactive?VIR_DOMAIN_XML_INACTIVE:0); + if (desc) + vshPrint(ctl, "%s", desc); + else + vshPrint(ctl, _("No description for domain: %s"), + virDomainGetName(dom)); + } + + ret = true; +cleanup: + VIR_FREE(desc_edited); + VIR_FREE(desc); + if (tmp) { + unlink(tmp); + VIR_FREE(tmp); + } + return ret; } /* @@ -15951,6 +16118,7 @@ static const vshCmdDef domManagementCmds[] = { {"migrate-getspeed", cmdMigrateGetMaxSpeed, opts_migrate_getspeed, info_migrate_getspeed, 0}, {"numatune", cmdNumatune, opts_numatune, info_numatune, 0}, + {"desc", cmdDesc, opts_desc, info_desc, 0}, {"reboot", cmdReboot, opts_reboot, info_reboot, 0}, {"reset", cmdReset, opts_reset, info_reset, 0}, {"restore", cmdRestore, opts_restore, info_restore, 0}, @@ -17671,6 +17839,44 @@ vshDomainStateReasonToString(int state, int reason) return N_("unknown"); } +/* extract note from domain xml */ +static char * +vshGetDomainDescription(vshControl *ctl, virDomainPtr dom, bool note, + unsigned int flags) +{ + char *desc = NULL; + char *domxml = NULL; + xmlDocPtr doc = NULL; + xmlXPathContextPtr ctxt = NULL; + + /* get domains xml description and extract the note */ + if (!(domxml = virDomainGetXMLDesc(dom, flags))) { + vshError(ctl, "%s", _("Failed to retrieve domain XML")); + goto cleanup; + } + doc = virXMLParseStringCtxt(domxml, + _("(domain_definition)"), + &ctxt); + if (!doc) { + vshError(ctl, "%s", _("Couldn't parse domain XML")); + goto cleanup; + } + if (note) + desc = virXPathString("string(./description[1]/@note)", ctxt); + else + desc = virXPathString("string(./description[1])", ctxt); + + if (!desc) + desc = vshStrdup(ctl, ""); + +cleanup: + VIR_FREE(domxml); + xmlXPathFreeContext(ctxt); + xmlFreeDoc(doc); + + return desc; +} + /* Return a non-NULL string representation of a typed parameter; exit * if we are out of memory. */ static char * diff --git a/tools/virsh.pod b/tools/virsh.pod index c88395b..afea430 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -279,7 +279,7 @@ The XML also show the NUMA topology information if available. Inject NMI to the guest. -=item B<list> [I<--inactive> | I<--all>] [I<--managed-save>] +=item B<list> [I<--inactive> | I<--all>] [I<--managed-save>] [I<--note>] Prints information about existing domains. If no options are specified it prints out information about running domains. @@ -350,6 +350,15 @@ If I<--managed-save> is specified, then domains that have managed save state (only possible if they are in the B<shut off> state) will instead show as B<saved> in the listing. +If I<--note> is specified, then the domain note is printed. The output then +the output looks as follows. + +B<virsh> list --note + Id Name State Note +----------------------------------------------- + 0 Domain-0 running Mailserver 1 + 2 fedora paused + =item B<freecell> [B<cellno> | I<--all>] Prints the available amount of memory on the machine or within a @@ -426,6 +435,25 @@ Define a domain from an XML <file>. The domain definition is registered but not started. If domain is already running, the changes will take effect on the next boot. +=item B<desc> [I<--live> | I<--persistent>] [I<--note>] [I<--edit>] + [I<--new_desc> New description or note message] + +Show or modify description and note for a domain. These values are user +fields that allow to store arbitrary textual data to allow easy identifiaction +of domains. Note is a short (maximum 40 characters) field. + +Flags I<--live> or I<--persistent> select wether this command works on live +or persistent definitions of the domain. By default both are infuenced, while +modifying and running definition is used while reading the note. + +Flag I<--edit> specifies that an editor with the contents of current description +or note should be opened and the contents save back afterwards. + +Flag I<--note> selects operation on the note field instead of description. + +If neither of I<--edit> and I<--new_desc> are specified the note or description +is displayed instead of being modified. + =item B<destroy> I<domain-id> Immediately terminate the domain domain-id. This doesn't give the domain -- 1.7.3.4
On 01/13/2012 11:17 AM, Peter Krempa wrote:
This patch adds a new command "desc" to show and modify notes and description for the domains using the new API.
This patch also adds a new flag for the "list" command to show notes in the domain list, to allow easy identification of VMs by storing a short description.
Example: virsh # list --note Id Name State Note ----------------------------------------------- 0 Domain-0 running Mailserver 1 2 fedora paused --- tools/virsh.c | 246 ++++++++++++++++++++++++++++++++++++++++++++++++++----- tools/virsh.pod | 30 +++++++- 2 files changed, 255 insertions(+), 21 deletions(-)
Looks like a great start!
+/* + * "desc" command for managing domain description and note + */ +static const vshCmdInfo info_desc[] = { + {"help", N_("show or set domain's description or note")}, + {"desc", N_("Allows to show or modify description or note of a domain.")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_desc[] = { + {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, + {"persistent", VSH_OT_BOOL, 0, N_("modify persistent state of domain")}, + {"live", VSH_OT_BOOL, 0, N_("modify note only for current instance")},
Should we prefer the names --live/--config/--current, rather than just --persistent/--live, for consistency with other recently added commands?
+ {"note", VSH_OT_BOOL, 0, N_("modify the note instead of description")}, + {"edit", VSH_OT_BOOL, 0, N_("open an editor to modify the description")}, + {"new_desc", VSH_OT_ARGV, 0, N_("message")},
Underscores are pain. I'd name this new-desc. I like the slick trick to make quoting optional, where I can use either: virsh desc domain --new_desc='my description' virsh desc domain my description
+ if (live) + flags |= VIR_DOMAIN_DESCRIPTION_LIVE; + if (inactive) + flags |= VIR_DOMAIN_DESCRIPTION_CONFIG; + if (!(inactive || live)) { + flags |= VIR_DOMAIN_DESCRIPTION_CONFIG; + if (state == VIR_DOMAIN_RUNNING || state == VIR_DOMAIN_PAUSED) + flags |= VIR_DOMAIN_DESCRIPTION_LIVE; + }
This last step isn't needed; the drivers already convert _CURRENT into the correct version, so virsh doesn't have to repeat that work.
+ + if (virBufferError(&buf)) { + vshPrint(ctl, "%s", _("Failed to collect new description/note")); + goto cleanup; + } + desc = virBufferContentAndReset(&buf); + + if (edit || desc) { + if (!desc) { + desc = vshGetDomainDescription(ctl, dom, note, + inactive?VIR_DOMAIN_XML_INACTIVE:0); + if (!desc) + goto cleanup; + }
This ignored --live; more on that below.
+ + if (edit) { + /* Create and open the temporary file. */ + tmp = editWriteToTempFile (ctl, desc);
Format - no space before function call (. Several instances in this function, and yes, I know it is from copy-and-paste.
+ + if (virDomainSetDescription(dom, desc, flags) < 0) { + vshError(ctl, "%s", + _("Failed to set new domain description")); + goto cleanup; + } + } else { + desc = vshGetDomainDescription(ctl, dom, note, + inactive?VIR_DOMAIN_XML_INACTIVE:0);
If the user did 'virsh desc domain --live --persistent', then you silently ignored --live; I'd almost rather have virsh error out (that is, --live and --persistent together make sense only if the user is providing a description - they don't even work together with --edit, since it is not obvious whether the editor should start from the live or from the config version).
+ if (desc) + vshPrint(ctl, "%s", desc); + else + vshPrint(ctl, _("No description for domain: %s"), + virDomainGetName(dom));
goto cleanup - this should be treated as an error condition (given that vshGetDomainDescription returns "", rather than NULL, if it successfully determined that the domain has no description).
@@ -15951,6 +16118,7 @@ static const vshCmdDef domManagementCmds[] = { {"migrate-getspeed", cmdMigrateGetMaxSpeed, opts_migrate_getspeed, info_migrate_getspeed, 0}, {"numatune", cmdNumatune, opts_numatune, info_numatune, 0}, + {"desc", cmdDesc, opts_desc, info_desc, 0},
Sort this line earlier.
+/* extract note from domain xml */ +static char * +vshGetDomainDescription(vshControl *ctl, virDomainPtr dom, bool note, + unsigned int flags) +{ + char *desc = NULL; + char *domxml = NULL; + xmlDocPtr doc = NULL; + xmlXPathContextPtr ctxt = NULL;
Here's where a getter function might make life easier (but you should still fall back to GetXMLDesc in case you are talking to older servers).
+ + /* get domains xml description and extract the note */ + if (!(domxml = virDomainGetXMLDesc(dom, flags))) { + vshError(ctl, "%s", _("Failed to retrieve domain XML")); + goto cleanup; + } + doc = virXMLParseStringCtxt(domxml, + _("(domain_definition)"), + &ctxt); + if (!doc) { + vshError(ctl, "%s", _("Couldn't parse domain XML")); + goto cleanup; + } + if (note) + desc = virXPathString("string(./description[1]/@note)", ctxt); + else + desc = virXPathString("string(./description[1])", ctxt);
If note is not present, but description is, should we fall back to a truncated version of description rather than nothing at all?
@@ -426,6 +435,25 @@ Define a domain from an XML <file>. The domain definition is registered but not started. If domain is already running, the changes will take effect on the next boot.
+=item B<desc> [I<--live> | I<--persistent>] [I<--note>] [I<--edit>] + [I<--new_desc> New description or note message] + +Show or modify description and note for a domain. These values are user +fields that allow to store arbitrary textual data to allow easy identifiaction +of domains. Note is a short (maximum 40 characters) field.
more on the characters vs. bytes distinction
+ +Flags I<--live> or I<--persistent> select wether this command works on live
s/wether/whether/
+or persistent definitions of the domain. By default both are infuenced, while
s/infuenced/influenced/
+modifying and running definition is used while reading the note.
I'm not quite sure that matches the code. That is, the code has to pass AFFECT_LIVE|AFFECT_CONFIG, and not 0, to affect both persistent and running setting. And if you switch to --live/--config/--current, that changes this paragraph even more.
+ +Flag I<--edit> specifies that an editor with the contents of current description +or note should be opened and the contents save back afterwards.
s/save/saved/
+ +Flag I<--note> selects operation on the note field instead of description. + +If neither of I<--edit> and I<--new_desc> are specified the note or description +is displayed instead of being modified. +
Overall, it sounds like a nice virsh addition. It will need some polish for v2, but shouldn't be too hard. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/13/2012 11:17 AM, Peter Krempa wrote:
This patchset adds ability to store, display and modify comments for domains to help administrators identify and store metadata to domains to allow easy identification.
A short description "note" was added as an attribute for the <description> element to hold a shorter description (limited to max 40 characters) to be shown along with lists of domains.
Do we really want to add a new XML attribute/element, or is it redundant with <description>? (By the way, I agree with Dan that a new element <title> is nicer than an attribute to <description>, if the answer is that yes, we really do want to distinguish between a length-limited field and an unlimited length description). I guess I'm 70/30 in favor of adding a new field, since we can put restrictions on the new field (max length, no newlines), which make it friendlier for reuse, while still leaving the existing description for its full power. But it would also be nice that if a domain has a description but not the new field, then the virsh code can truncate the description field and provide that as though it had been the note field.
docs/formatdomain.html.in | 7 +- docs/schemas/domaincommon.rng | 14 ++- include/libvirt/libvirt.h.in | 15 +++ src/conf/domain_conf.c | 27 ++++- src/conf/domain_conf.h | 1 + src/driver.h | 5 + src/libvirt.c | 47 ++++++++ src/libvirt_public.syms | 5 + src/qemu/qemu_driver.c | 70 ++++++++++++ src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 9 ++- tools/virsh.c | 246 +++++++++++++++++++++++++++++++++++++---- tools/virsh.pod | 30 +++++-
Missing a tests addition; I'd suggest something in qemuxml2xmltest.c that proves we can parse and regenerate the new XML. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Peter Krempa