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@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
+-------------
+
+**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``.
+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.
+
+
 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,
+     .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"));
+        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/>"));
+            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;
+}
+
+
 /*
  * "memtune" command
  */
@@ -14995,6 +15143,12 @@ const vshCmdDef domManagementCmds[] = {
      .info = info_update_device,
      .flags = 0
     },
+    {.name = "update-memory",
+     .handler = cmdUpdateMemory,
+     .opts = opts_update_memory,
+     .info = info_update_memory,
+     .flags = 0
+    },
     {.name = "vcpucount",
      .handler = cmdVcpucount,
      .opts = opts_vcpucount,
-- 
2.26.2

On 1/22/21 9:50 AM, 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.

I think you meant:

"... want to change <myElement/>? A new --my-element argument ..."



Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

> 
> Signed-off-by: Michal Privoznik <mprivozn@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
> +-------------
> +
> +**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``.
> +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.
> +
> +
>   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,
> +     .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"));
> +        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/>"));
> +            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;
> +}
> +
> +
>   /*
>    * "memtune" command
>    */
> @@ -14995,6 +15143,12 @@ const vshCmdDef domManagementCmds[] = {
>        .info = info_update_device,
>        .flags = 0
>       },
> +    {.name = "update-memory",
> +     .handler = cmdUpdateMemory,
> +     .opts = opts_update_memory,
> +     .info = info_update_memory,
> +     .flags = 0
> +    },
>       {.name = "vcpucount",
>        .handler = cmdVcpucount,
>        .opts = opts_vcpucount,
> 

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@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?

> +-------------
> +
> +**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.


> +
> +
>  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.

> +     .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?


> +        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.

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@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