[PATCH 09/10] virsh: Introduce update-memory command
Michal Privoznik
mprivozn at redhat.com
Wed Feb 3 16:27:03 UTC 2021
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 at 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
More information about the libvir-list
mailing list