On 2/2/21 2:41 PM, Peter Krempa wrote:
On Fri, Jan 22, 2021 at 13:50:31 +0100, Michal Privoznik wrote:
New 'update-memory' command is introduced which aims on making it
user friendly to change <memory/> device. So far I just need to
change <requested/> so I'm introducing --requested-size only; but
the idea is that this is extensible for other cases too. For
instance, want to change <myElement/>? Nnew --my-element
argument can be easily introduced.
Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
---
docs/manpages/virsh.rst | 31 ++++++++
tools/virsh-domain.c | 154 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 185 insertions(+)
diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
index e3afa48f7b..32639e34ff 100644
--- a/docs/manpages/virsh.rst
+++ b/docs/manpages/virsh.rst
@@ -4891,6 +4891,37 @@ results as some fields may be autogenerated and thus match devices other than
expected.
+update-memory
update-memory-device-perhaps?
Okay.
+-------------
+
+**Syntax:**
+
+::
+
+ update-memory domain [--print-xml] [--alias alias]
+ [[--live] [--config] | [--current]]
+ [--requested-size size]
+
+Update values for a ``<memory/>`` device. Not to be confused with overall
+domain memory which is tuned via ``setmem`` and ``setmaxmem``.
So that you don't have to add this disclaimer?
+This command finds ``<memory/>`` device inside given *domain*, changes
+requested values and passes updated device XML to daemon. If *--print-xml* is
+specified then the device is not changed, but the updated device XML is printed
+to stdout. If there are more than one ``<memory/>`` devices in *domain* use
+*--alias* to select the desired one.
+
+If *--live* is specified, affect a running domain.
+If *--config* is specified, affect the next startup of a persistent guest.
+If *--current* is specified, it is equivalent to either *--live* or
+*--config*, depending on the current state of the guest.
+Both *--live* and *--config* flags may be given, but *--current* is
+exclusive. Not specifying any flag is the same as specifying *--current*.
+
+If *--requested-size* is specified then ``<requested/>`` under memory target is
+changed to requested *size* (as scaled integer, see ``NOTES`` above). It
+defaults to kibibytes if no suffix is provided.
... this document doesn't mention that it works only for the property of
virtio-mem. Users of virsh tend to not read other docs, so please add
it.
I can do that, but virsh errors out if it didn't find any <requested/>,
like this:
virsh # update-memory gentoo --alias virtiopmem0 --print-xml
--requested-size 5
error: virtio-mem device is missing <requested/>
Okay, the error message is misleading a bit (will fix it), because I was
trying to modify virtio-pmem.
How badly do we want to protect users from themselves?
+
+
change-media
------------
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 9746117bdb..0b32e6f408 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -9128,6 +9128,154 @@ cmdSetmaxmem(vshControl *ctl, const vshCmd *cmd)
return ret;
}
+
+/*
+ * "update-memory" command
+ */
+static const vshCmdInfo info_update_memory[] = {
+ {.name = "help",
+ .data = N_("update memory device of a domain")
+ },
+ {.name = "desc",
+ .data = N_("Update values of a memory device of a domain")
+ },
+ {.name = NULL}
+};
+
+static const vshCmdOptDef opts_update_memory[] = {
+ VIRSH_COMMON_OPT_DOMAIN_FULL(0),
+ VIRSH_COMMON_OPT_DOMAIN_CONFIG,
+ VIRSH_COMMON_OPT_DOMAIN_LIVE,
+ VIRSH_COMMON_OPT_DOMAIN_CURRENT,
+ {.name = "print-xml",
+ .type = VSH_OT_BOOL,
+ .help = N_("print updated memory device XML instead of executing the change")
+ },
+ {.name = "alias",
+ .type = VSH_OT_STRING,
+ .completer = virshDomainDeviceAliasCompleter,
This completes also non-memory devices.
Yes, and I am okay with that. Alias is optional and needed only if two
or more <memory/> devices exist inside domain (regardless of their
model). We already allow completion of mutually exclusive --options
(because the exclusivity is not expressed in --options definition). We
could have .completer_flags as an ORed set of virDomainDeviceType,
except not really because that's an enum with continuous values and not
a bitmask. I could write a new completer, but if we want to do that for
every device (eventually) we would need ~20 different completers. Waste
of time.
Again, I don't think we should guard users from shooting themselves into
foot.
+ .help = N_("memory device alias"),
+ },
+ {.name = "requested-size",
+ .type = VSH_OT_INT,
+ .help = N_("new value of <requested/> size, as scaled integer (default KiB)")
+ },
+ {.name = NULL}
+};
+
+static int
+virshGetUpdatedMemoryXML(char **updatedMemoryXML,
+ vshControl *ctl,
+ const vshCmd *cmd,
+ virDomainPtr dom,
+ unsigned int flags)
+{
+ const char *alias = NULL;
+ g_autoptr(xmlDoc) doc = NULL;
+ g_autoptr(xmlXPathContext) ctxt = NULL;
+ g_autofree char *xpath = NULL;
+ int nmems;
+ g_autofree xmlNodePtr *mems = NULL;
+ g_autoptr(xmlBuffer) xmlbuf = NULL;
+ unsigned int domainXMLFlags = 0;
+
+ if (flags & VIR_DOMAIN_AFFECT_CONFIG)
+ domainXMLFlags |= VIR_DOMAIN_XML_INACTIVE;
+
+ if (virshDomainGetXMLFromDom(ctl, dom, domainXMLFlags, &doc, &ctxt) < 0)
+ return -1;
+
+ if (vshCommandOptStringReq(ctl, cmd, "alias", &alias) < 0)
+ return -1;
+
+ if (alias) {
+ xpath = g_strdup_printf("/domain/devices/memory[./alias/@name='%s']", alias);
+ } else {
+ xpath = g_strdup("/domain/devices/memory");
+ }
+
+ nmems = virXPathNodeSet(xpath, ctxt, &mems);
+ if (nmems < 0) {
+ vshSaveLibvirtError();
+ return -1;
+ } else if (nmems == 0) {
+ vshError(ctl, _("no memory device found"));
+ return -1;
+ } else if (nmems > 1) {
+ vshError(ctl, _("multiple memory devices found, use --alias to select one"));
So if you don't have useraliases, you can't use this for inactive XML
with 2 virtio mem?
Updating inactive XML is not even implemented yet, so can't use it even
if you have only one virtio mem. If we will want that, then we need
--address to match the device address because that is the only unique
attribute. I'd leave it for future work.
+ return -1;
+ }
+
+ ctxt->node = mems[0];
+
+ if (vshCommandOptBool(cmd, "requested-size")) {
+ xmlNodePtr requestedSizeNode;
+ g_autofree char *kibibytesStr = NULL;
+ unsigned long long bytes = 0;
+ unsigned long kibibytes = 0;
+
+ if (vshCommandOptScaledInt(ctl, cmd, "requested-size", &bytes, 1024, ULLONG_MAX) < 0)
+ return -1;
+ kibibytes = VIR_DIV_UP(bytes, 1024);
+
+ requestedSizeNode = virXPathNode("./target/requested", ctxt);
+
+ if (!requestedSizeNode) {
+ vshError(ctl, _("virtio-mem device is missing <requested/>"));
Here you mention virtio-mem.
+ return -1;
+ }
+
+ kibibytesStr = g_strdup_printf("%lu", kibibytes);
+ xmlNodeSetContent(requestedSizeNode, BAD_CAST kibibytesStr);
+ }
+
+ if (!(*updatedMemoryXML = virXMLNodeToString(doc, mems[0]))) {
+ vshSaveLibvirtError();
+ return -1;
+ }
+
+ return 0;
+}
+
+static bool
+cmdUpdateMemory(vshControl *ctl, const vshCmd *cmd)
+{
+ virDomainPtr dom;
+ bool ret = false;
+ bool config = vshCommandOptBool(cmd, "config");
+ bool live = vshCommandOptBool(cmd, "live");
+ bool current = vshCommandOptBool(cmd, "current");
+ g_autofree char *updatedMemoryXML = NULL;
+ unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
+
+ VSH_EXCLUSIVE_OPTIONS_VAR(current, live);
+ VSH_EXCLUSIVE_OPTIONS_VAR(current, config);
+
+ if (config)
+ flags |= VIR_DOMAIN_AFFECT_CONFIG;
+ if (live)
+ flags |= VIR_DOMAIN_AFFECT_LIVE;
+
+ if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
+ return false;
+
+ if (virshGetUpdatedMemoryXML(&updatedMemoryXML, ctl, cmd, dom, flags) < 0)
+ goto cleanup;
+
+ if (vshCommandOptBool(cmd, "print-xml")) {
+ vshPrint(ctl, "%s", updatedMemoryXML);
+ } else {
+ if (virDomainUpdateDeviceFlags(dom, updatedMemoryXML, flags) < 0)
+ goto cleanup;
+ }
+
+ ret = true;
+ cleanup:
+ virshDomainFree(dom);
+ return ret;
You can use g_autoptr(virshDomain) dom = NULL; on top instead of the
cleanup section.
Yup.
Michal
