From 3d21e502aebaf8415c466a62c070d7880a00cf2e Mon Sep 17 00:00:00 2001
From: Marius Vollmer <marius.vollmer@gmail.com>
Date: Tue, 21 May 2024 09:35:26 +0300
Subject: [PATCH] vm: Support for VM titles and descriptions

Based on work by @Britz, thanks a lot!
---
 src/components/vm/vmActions.jsx      |  4 +-
 src/components/vm/vmDetailsPage.jsx  |  6 ++-
 src/components/vm/vmRenameDialog.jsx | 64 ++++++++++++++++++---------
 src/components/vms/hostvmslist.jsx   | 14 +++---
 src/libvirt-xml-parse.js             |  6 +++
 src/libvirtApi/domain.js             | 27 ++++++++++++
 test/check-machines-lifecycle        | 65 +++++++++++++++++++++++++---
 test/reference                       |  2 +-
 8 files changed, 150 insertions(+), 38 deletions(-)

diff --git a/src/components/vm/vmActions.jsx b/src/components/vm/vmActions.jsx
index dd731df30..009022648 100644
--- a/src/components/vm/vmActions.jsx
+++ b/src/components/vm/vmActions.jsx
@@ -445,9 +445,7 @@ const VmActions = ({ vm, vms, onAddErrorNotification, isDetailsPage }) => {
         dropdownItems.push(
             <DropdownItem key={`${id}-rename`}
                           id={`${id}-rename`}
-                          onClick={() => Dialogs.show(<RenameDialog vmName={vm.name}
-                                                                    vmId={vm.id}
-                                                                    connectionName={vm.connectionName} />)}>
+                          onClick={() => Dialogs.show(<RenameDialog vm={vm} />)}>
                 {_("Rename")}
             </DropdownItem>
         );
diff --git a/src/components/vm/vmDetailsPage.jsx b/src/components/vm/vmDetailsPage.jsx
index 98fbfeb12..cdf907da7 100644
--- a/src/components/vm/vmDetailsPage.jsx
+++ b/src/components/vm/vmDetailsPage.jsx
@@ -67,7 +67,7 @@ export const VmDetailsPage = ({
     const vmActionsPageSection = (
         <PageSection className="actions-pagesection" variant={PageSectionVariants.light} isWidthLimited>
             <div className="vm-top-panel" data-vm-transient={!vm.persistent}>
-                <h2 className="vm-name">{vm.name}</h2>
+                <h2 className="vm-name">{vm.title ? vm.title + " (" + vm.name + ")" : vm.name}</h2>
                 <VmActions vm={vm}
                            config={config}
                            onAddErrorNotification={onAddErrorNotification}
@@ -75,6 +75,10 @@ export const VmDetailsPage = ({
                 <VmNeedsShutdown vm={vm} />
                 <VmUsesSpice vm={vm} />
             </div>
+            {
+                vm.description &&
+                    <div className="vm-description">{vm.description.split("\n").map((p, i) => <p key={i}>{p}</p>)}</div>
+            }
         </PageSection>
     );
 
diff --git a/src/components/vm/vmRenameDialog.jsx b/src/components/vm/vmRenameDialog.jsx
index b9ab48619..0bff2231a 100644
--- a/src/components/vm/vmRenameDialog.jsx
+++ b/src/components/vm/vmRenameDialog.jsx
@@ -23,47 +23,57 @@ import { Button } from "@patternfly/react-core/dist/esm/components/Button";
 import { Form, FormGroup } from "@patternfly/react-core/dist/esm/components/Form";
 import { Modal } from "@patternfly/react-core/dist/esm/components/Modal";
 import { TextInput } from "@patternfly/react-core/dist/esm/components/TextInput";
+import { TextArea } from "@patternfly/react-core/dist/esm/components/TextArea";
 
 import { FormHelper } from 'cockpit-components-form-helper.jsx';
 import { ModalError } from 'cockpit-components-inline-notification.jsx';
 import { useDialogs } from 'dialogs.jsx';
 
 import { isObjectEmpty } from '../../helpers.js';
-import { domainRename } from '../../libvirtApi/domain.js';
+import { domainRename, domainSetMetadata } from '../../libvirtApi/domain.js';
 
 const _ = cockpit.gettext;
 
-export const RenameDialog = ({ vmName, vmId, connectionName }) => {
+export const RenameDialog = ({ vm }) => {
     const Dialogs = useDialogs();
-    const [newName, setNewName] = useState(vmName);
+    const [name, setName] = useState(vm.name);
+    const [fullName, setFullName] = useState(vm.title || "");
+    const [description, setDescription] = useState(vm.description || "");
     const [error, dialogErrorSet] = useState({});
     const [submitted, setSubmitted] = useState(false);
 
-    function onRename() {
+    async function onRename() {
         setSubmitted(true);
 
-        if (!newName)
+        if (!name)
             return;
 
-        return domainRename({ connectionName, id: vmId, newName })
-                .then(() => {
-                    Dialogs.close();
-                    // If we are on the VMs details page change the URL to reflect the new name after the rename operation succeeded
-                    if (cockpit.location.path.length > 0)
-                        cockpit.location.go(["vm"], { ...cockpit.location.options, name: newName, connection: connectionName });
-                }, exc => {
-                    dialogErrorSet({ dialogError: cockpit.format(_("Failed to rename VM $0"), vmName), dialogErrorDetail: exc.message });
-                });
+        try {
+            await domainSetMetadata(vm, { title: fullName, description });
+            if (name != vm.name)
+                await domainRename({ connectionName: vm.connectionName, id: vm.id, newName: name });
+            Dialogs.close();
+
+            // If we are on the VMs details page change the URL to
+            // reflect the new name after the rename operation
+            // succeeded
+            if (name != vm.name && cockpit.location.path.length > 0)
+                cockpit.location.go(["vm"], { ...cockpit.location.options, name, connection: vm.connectionName });
+        } catch (exc) {
+            dialogErrorSet({
+                dialogError: cockpit.format(_("Failed to rename VM $0"), vm.name),
+                dialogErrorDetail: exc.message
+            });
+        }
     }
 
     return (
         <Modal position="top" variant="small" isOpen onClose={Dialogs.close}
-           title={cockpit.format(_("Rename VM $0"), vmName)}
+           title={cockpit.format(_("Rename VM $0"), vm.name)}
            footer={
                <>
                    <Button variant='primary'
                            id="rename-dialog-confirm"
-                           isDisabled={!newName}
                            onClick={onRename}>
                        {_("Rename")}
                    </Button>
@@ -78,13 +88,25 @@ export const RenameDialog = ({ vmName, vmId, connectionName }) => {
             }}
             isHorizontal>
                 {!isObjectEmpty(error) && <ModalError dialogError={error.dialogError} dialogErrorDetail={error.dialogErrorDetail} />}
-                <FormGroup label={_("New name")}
+                <FormGroup label={_("Name")}
                            fieldId="rename-dialog-new-name">
                     <TextInput id='rename-dialog-new-name'
-                               validated={submitted && !newName ? "error" : "default"}
-                               value={newName}
-                               onChange={(_, value) => setNewName(value)} />
-                    <FormHelper helperTextInvalid={submitted && !newName && _("New name must not be empty")} />
+                               validated={submitted && !name ? "error" : "default"}
+                               value={name}
+                               onChange={(_, value) => setName(value)} />
+                    <FormHelper helperTextInvalid={submitted && !name && _("Name must not be empty")} />
+                </FormGroup>
+                <FormGroup label={_("Full name")}
+                           fieldId="rename-dialog-full-name">
+                    <TextInput id='rename-dialog-full-name'
+                               value={fullName}
+                               onChange={(_, value) => setFullName(value)} />
+                </FormGroup>
+                <FormGroup label={_("Description")}
+                           fieldId="rename-dialog-description">
+                    <TextArea id='rename-dialog-description'
+                              value={description}
+                              onChange={(_, value) => setDescription(value)} />
                 </FormGroup>
             </Form>
         </Modal>
diff --git a/src/components/vms/hostvmslist.jsx b/src/components/vms/hostvmslist.jsx
index b59bff15b..2b2959c73 100644
--- a/src/components/vms/hostvmslist.jsx
+++ b/src/components/vms/hostvmslist.jsx
@@ -158,10 +158,11 @@ const HostVmsList = ({ vms, config, ui, storagePools, actions, networks, onAddEr
                                 <ListingTable aria-label={_("Virtual machines")}
                             variant='compact'
                             columns={[
-                                { title: _("Name"), header: true, props: { width: 25 } },
-                                { title: _("Connection"), props: { width: 25 } },
-                                { title: _("State"), props: { width: 25 } },
-                                { title: "", props: { width: 25, "aria-label": _("Actions") } },
+                                { title: _("Name"), header: true, props: { width: 20 } },
+                                { title: _("Full name"), props: { width: 20 } },
+                                { title: _("Connection"), props: { width: 20 } },
+                                { title: _("State"), props: { width: 20 } },
+                                { title: "", props: { width: 20, "aria-label": _("Actions") } },
                             ]}
                             emptyCaption={_("No VM is running or defined on this host")}
                             rows={ combinedVmsFiltered
@@ -186,8 +187,11 @@ const HostVmsList = ({ vms, config, ui, storagePools, actions, networks, onAddEr
                                                               isDisabled={vm.isUi && !vm.createInProgress}
                                                               component="a"
                                                               href={'#' + cockpit.format("vm?name=$0&connection=$1", encodeURIComponent(vm.name), vm.connectionName)}
-                                                              className="vm-list-item-name">{vm.name}</Button>
+                                                              className="vm-list-item-name">
+                                                        {vm.name}
+                                                    </Button>
                                                 },
+                                                { title: vm.title },
                                                 { title: rephraseUI('connections', vm.connectionName) },
                                                 {
                                                     title: (
diff --git a/src/libvirt-xml-parse.js b/src/libvirt-xml-parse.js
index 74492abc0..3591a689a 100644
--- a/src/libvirt-xml-parse.js
+++ b/src/libvirt-xml-parse.js
@@ -245,6 +245,9 @@ export function parseDomainDumpxml(connectionName, domXml, objPath) {
     const metadataElem = getSingleOptionalElem(domainElem, "metadata");
 
     const name = domainElem.getElementsByTagName("name")[0].childNodes[0].nodeValue;
+    const uuid = domainElem.getElementsByTagName("uuid")[0].childNodes[0].nodeValue;
+    const title = domainElem.getElementsByTagName("title")[0]?.childNodes[0]?.nodeValue;
+    const description = domainElem.getElementsByTagName("description")[0]?.childNodes[0]?.nodeValue;
     const id = objPath;
     const osType = osTypeElem.childNodes[0].nodeValue;
     const osBoot = parseDumpxmlForOsBoot(osBootElems);
@@ -292,7 +295,10 @@ export function parseDomainDumpxml(connectionName, domXml, objPath) {
 
     return {
         connectionName,
+        uuid,
         name,
+        title,
+        description,
         id,
         osType,
         osBoot,
diff --git a/src/libvirtApi/domain.js b/src/libvirtApi/domain.js
index 5c9aa0dfd..3a3f7a766 100644
--- a/src/libvirtApi/domain.js
+++ b/src/libvirtApi/domain.js
@@ -890,6 +890,33 @@ export function domainSendNMI({
     return call(connectionName, objPath, 'org.libvirt.Domain', 'InjectNMI', [0], { timeout, type: 'u' });
 }
 
+function shlex_quote(str) {
+    // yay, command line apis...
+    return "'" + str.replaceAll("'", "'\"'\"'") + "'";
+}
+
+async function domainSetXML(vm, option, values) {
+    const opts = { err: "message" };
+    if (vm.connectionName === 'system')
+        opts.superuser = 'try';
+
+    // We don't pass the arguments for virt-xml through a shell, but
+    // virt-xml does its own parsing with the Python shlex module. So
+    // we need to do the equivalent of shlex.quote here.
+
+    const args = [];
+    for (const key in values)
+        args.push(shlex_quote(key + '=' + values[key]))
+
+    await cockpit.spawn([
+        'virt-xml', '-c', `qemu:///${vm.connectionName}`, '--' + option, args.join(','), vm.uuid, '--edit'
+    ], opts);
+}
+
+export async function domainSetMetadata(vm, values) {
+    await domainSetXML(vm, "metadata", values);
+}
+
 export function domainSetCpuMode({
     name,
     id: objPath,
diff --git a/test/check-machines-lifecycle b/test/check-machines-lifecycle
index 06e08ada6..bec00b786 100755
--- a/test/check-machines-lifecycle
+++ b/test/check-machines-lifecycle
@@ -349,22 +349,73 @@ class TestMachinesLifecycle(machineslib.VirtualMachinesCase):
         self.waitVmRow("new")
         self.waitVmRow("old", "system", False)
 
-        # Rename to itself and expect error
+        self.goToVmPage("new")
         self.performAction("new", "rename")
 
-        b.set_input_text("#rename-dialog-new-name", "new")
+        # Empty name should be an error
+
+        b.set_input_text("#rename-dialog-new-name", "")
         b.click("#rename-dialog-confirm")
-        b.wait_in_text(".pf-v5-c-modal-box__body .pf-v5-c-alert.pf-m-danger", "Can't rename domain to itself")
+        b.wait_in_text(".pf-v5-c-helper-text__item.pf-m-error", "Name must not be empty")
 
-        self.goToVmPage("new")
-        self.performAction("new", "rename")
+        # Test errors from libvirt
 
-        b.set_input_text("#rename-dialog-new-name", "test%")
+        b.set_input_text("#rename-dialog-new-name", "/")
+        b.click("#rename-dialog-confirm")
+        b.wait_in_text(".pf-v5-c-alert__description", "name / cannot contain '/'")
+
+        # Set full name and description and use non-ASCII characters
+
+        # Non-ascii chars, unbalanced quotes, backslash, multiple lines
+        desc1 = '"Döscrü\\ptiän \'\'\' كرة القدم'
+        desc2 = 'Second <b>line</b>'
+        desc = desc1 + "\n" + desc2
+
+        b.set_input_text("#rename-dialog-new-name", 'test%')
+        # This is an attempted injection for the virt-xml invocation
+        b.set_input_text("#rename-dialog-full-name", '"full name",blah=yo')
+        # set_input_text can't handle the newline, but we need it for the event.
+        b.set_val("#rename-dialog-description", desc[:-1])
+        b.set_input_text("#rename-dialog-description", desc[-1], append=True, value_check=False)
         b.click("#rename-dialog-confirm")
         b.wait(lambda: "vm?name=test%25&connection=system" in b.eval_js("window.location.href"))
-        b.wait_text("h2.vm-name", "test%")
+        b.wait_text("h2.vm-name", '"full name",blah=yo (test%)')
+        b.wait_text(".vm-description p:nth-child(1)", desc1)
+        b.wait_text(".vm-description p:nth-child(2)", desc2)
         b.wait_not_present("#navbar-oops")
 
+        # Check that full name and description are picked up correctly by rename dialog
+
+        self.performAction("test\\%", "rename")
+        b.wait_val("#rename-dialog-new-name", 'test%')
+        b.wait_val("#rename-dialog-full-name", '"full name",blah=yo')
+        b.wait_val("#rename-dialog-description", desc)
+
+        # Rename machine to "--", which is allowed by libvirt and has
+        # the potential of breaking naive command line interactions.
+
+        b.set_input_text("#rename-dialog-new-name", '--')
+        b.set_input_text("#rename-dialog-full-name", 'Can you handle this?')
+        b.click("#rename-dialog-confirm")
+        b.wait(lambda: "vm?name=--&connection=system" in b.eval_js("window.location.href"))
+
+        # Rename back to something sane
+
+        self.performAction("--", "rename")
+        b.set_input_text("#rename-dialog-new-name", 'srv-1')
+        b.set_input_text("#rename-dialog-full-name", 'Server I')
+        b.set_input_text("#rename-dialog-description", '')
+        b.click("#rename-dialog-confirm")
+        b.wait(lambda: "vm?name=srv-1&connection=system" in b.eval_js("window.location.href"))
+
+        b.wait_text("h2.vm-name", 'Server I (srv-1)')
+        b.wait_not_present(".vm-description")
+
+        # Check overview
+        self.goToMainPage()
+        self.waitVmRow("srv-1")
+        b.wait_text('td[data-label="Full name"]', "Server I")
+
     def testDelete(self):
         b = self.browser
         m = self.machine
diff --git a/test/reference b/test/reference
index ba24ad2a3..6449e8369 160000
--- a/test/reference
+++ b/test/reference
@@ -1 +1 @@
-Subproject commit ba24ad2a36504e1bd930bc5b3593c912d58568fc
+Subproject commit 6449e836983c1d2915222a197998acda81ec22e6