10:27 a.m.
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(a)redhat.com>
> ---
> 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