Re: [libvirt] [PATCH 9/9] Modify virsh commands

Daniel P. Berrange wrote: > On Thu, Jan 14, 2010 at 10:42:46AM -0700, Jim Fehlig wrote: > >> Change all virsh commands that invoke virDomain{Attach,Detach}Device() >> to use virDomain{Attach,Detach}DeviceFlags() instead. >> >> Add a "--persistent" flag to these virsh commands, allowing user to >> specify that the domain persisted config be modified as well. >> > > > > >> --- >> tools/virsh.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++------ >> 1 files changed, 49 insertions(+), 6 deletions(-) >> >> diff --git a/tools/virsh.c b/tools/virsh.c >> index 1fae5e6..a082b89 100644 >> --- a/tools/virsh.c >> +++ b/tools/virsh.c >> @@ -6285,6 +6285,7 @@ static const vshCmdInfo info_attach_device[] = { >> static const vshCmdOptDef opts_attach_device[] = { >> {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("domain name, id or uuid")}, >> {"file", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("XML file")}, >> + {"persistent", VSH_OT_BOOL, 0, gettext_noop("persist device attachment")}, >> {NULL, 0, 0, NULL} >> }; >> >> @@ -6296,6 +6297,7 @@ cmdAttachDevice(vshControl *ctl, const vshCmd *cmd) >> char *buffer; >> int ret = TRUE; >> int found; >> + int flags = VIR_DOMAIN_DEVICE_MODIFY_CURRENT; >> >> if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) >> return FALSE; >> @@ -6309,13 +6311,18 @@ cmdAttachDevice(vshControl *ctl, const vshCmd *cmd) >> virDomainFree(dom); >> return FALSE; >> } >> + if (vshCommandOptBool(cmd, "persistent")) >> + flags |= VIR_DOMAIN_DEVICE_MODIFY_CONFIG; >> >> if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) { >> virDomainFree(dom); >> return FALSE; >> } >> >> - ret = virDomainAttachDevice(dom, buffer); >> + if (virDomainIsActive(dom)) >> + flags |= VIR_DOMAIN_DEVICE_MODIFY_LIVE; >> + >> + ret = virDomainAttachDeviceFlags(dom, buffer, flags); >> VIR_FREE(buffer); >> >> if (ret < 0) { >> > > > This has the same subtle compatability problem that the public API > entry point suffers from. New virsh won't be able to modify guests > from an existing libvirtd. I think that if flags == 0, then we should > use the existing API method, and only use the new virDomainAttachDeviceFlags > if flags != 0. I think we probably want to default to 0, and only set > the VIR_DOMAIN_DEVICE_MODIFY_LIVE flag if a '--live' flag is given. > Basically we need to try & ensure compatability with existing operation > if at all possible > The existing behavior is essentially VIR_DOMAIN_DEVICE_MODIFY_LIVE. qemu fails the operation if domain is inactive. Attach works on inactive Xen domains, but detach does not. vbox has an impl for inactive domains, but I haven't tested it. I kept the existing behavior by only calling vir{Attach,Detach}DeviceFlags() only when the new virsh flag "persistent" is specified. Revised patch below. Thanks, Jim

commit 4e52e0d49d96958facab3e99b6b6f8519d2d9c76 Author: Jim Fehlig <jfehlig(a)novell.com&gt; Date: Wed Jan 13 18:54:58 2010 -0700 Modify virsh commands Change all virsh commands that invoke virDomain{Attach,Detach}Device() to use virDomain{Attach,Detach}DeviceFlags() instead. Add a "--persistent" flag to these virsh commands, allowing user to specify that the domain persisted config be modified as well. V2: Only invoke virDomain{Attach,Detach}DeviceFlags() if "--persistent" flag is specified. Otherwise invoke virDomain{Attach,Detach}Device() to retain current behavior. diff --git a/tools/virsh.c b/tools/virsh.c index 1fae5e6..0763dcc 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -6285,6 +6285,7 @@ static const vshCmdInfo info_attach_device[] = { static const vshCmdOptDef opts_attach_device[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("domain name, id or uuid")}, {"file", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("XML file")}, + {"persistent", VSH_OT_BOOL, 0, gettext_noop("persist device attachment")}, {NULL, 0, 0, NULL} }; @@ -6296,6 +6297,7 @@ cmdAttachDevice(vshControl *ctl, const vshCmd *cmd) char *buffer; int ret = TRUE; int found; + unsigned int flags; if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) return FALSE; @@ -6315,7 +6317,14 @@ cmdAttachDevice(vshControl *ctl, const vshCmd *cmd) return FALSE; } - ret = virDomainAttachDevice(dom, buffer); + if (vshCommandOptBool(cmd, "persistent")) { + flags = VIR_DOMAIN_DEVICE_MODIFY_CONFIG; + if (virDomainIsActive(dom) == 1) + flags |= VIR_DOMAIN_DEVICE_MODIFY_LIVE; + ret = virDomainAttachDeviceFlags(dom, buffer, flags); + } else { + ret = virDomainAttachDevice(dom, buffer); + } VIR_FREE(buffer); if (ret < 0) { @@ -6343,6 +6352,7 @@ static const vshCmdInfo info_detach_device[] = { static const vshCmdOptDef opts_detach_device[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("domain name, id or uuid")}, {"file", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("XML file")}, + {"persistent", VSH_OT_BOOL, 0, gettext_noop("persist device detachment")}, {NULL, 0, 0, NULL} }; @@ -6354,6 +6364,7 @@ cmdDetachDevice(vshControl *ctl, const vshCmd *cmd) char *buffer; int ret = TRUE; int found; + unsigned int flags; if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) return FALSE; @@ -6373,7 +6384,14 @@ cmdDetachDevice(vshControl *ctl, const vshCmd *cmd) return FALSE; } - ret = virDomainDetachDevice(dom, buffer); + if (vshCommandOptBool(cmd, "persistent")) { + flags = VIR_DOMAIN_DEVICE_MODIFY_CONFIG; + if (virDomainIsActive(dom) == 1) + flags |= VIR_DOMAIN_DEVICE_MODIFY_LIVE; + ret = virDomainDetachDeviceFlags(dom, buffer, flags); + } else { + ret = virDomainDetachDevice(dom, buffer); + } VIR_FREE(buffer); if (ret < 0) { @@ -6405,6 +6423,7 @@ static const vshCmdOptDef opts_attach_interface[] = { {"target", VSH_OT_DATA, 0, gettext_noop("target network name")}, {"mac", VSH_OT_DATA, 0, gettext_noop("MAC address")}, {"script", VSH_OT_DATA, 0, gettext_noop("script used to bridge network interface")}, + {"persistent", VSH_OT_BOOL, 0, gettext_noop("persist interface attachment")}, {NULL, 0, 0, NULL} }; @@ -6415,6 +6434,7 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd) char *mac, *target, *script, *type, *source; int typ, ret = FALSE; char *buf = NULL, *tmp = NULL; + unsigned int flags; if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) goto cleanup; @@ -6489,13 +6509,22 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd) if (!buf) goto cleanup; strcat(buf, " </interface>\n"); - if (virDomainAttachDevice(dom, buf)) { - goto cleanup; + if (vshCommandOptBool(cmd, "persistent")) { + flags = VIR_DOMAIN_DEVICE_MODIFY_CONFIG; + if (virDomainIsActive(dom) == 1) + flags |= VIR_DOMAIN_DEVICE_MODIFY_LIVE; + ret = virDomainAttachDeviceFlags(dom, buf, flags); } else { - vshPrint(ctl, "%s", _("Interface attached successfully\n")); + ret = virDomainAttachDevice(dom, buf); } - ret = TRUE; + if (ret != 0) { + vshError(ctl, _("Failed to attach interface")); + ret = FALSE; + } else { + vshPrint(ctl, "%s", _("Interface attached successfully\n")); + ret = TRUE; + } cleanup: if (dom) @@ -6518,6 +6547,7 @@ static const vshCmdOptDef opts_detach_interface[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("domain name, id or uuid")}, {"type", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("network interface type")}, {"mac", VSH_OT_STRING, 0, gettext_noop("MAC address")}, + {"persistent", VSH_OT_BOOL, 0, gettext_noop("persist interface detachment")}, {NULL, 0, 0, NULL} }; @@ -6534,6 +6564,7 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd) char *doc, *mac =NULL, *type; char buf[64]; int i = 0, diff_mac, ret = FALSE; + unsigned int flags; if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) goto cleanup; @@ -6605,10 +6636,21 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd) goto cleanup; } - ret = virDomainDetachDevice(dom, (char *)xmlBufferContent(xml_buf)); - if (ret != 0) + if (vshCommandOptBool(cmd, "persistent")) { + flags = VIR_DOMAIN_DEVICE_MODIFY_CONFIG; + if (virDomainIsActive(dom) == 1) + flags |= VIR_DOMAIN_DEVICE_MODIFY_LIVE; + ret = virDomainDetachDeviceFlags(dom, + (char *)xmlBufferContent(xml_buf), + flags); + } else { + ret = virDomainDetachDevice(dom, (char *)xmlBufferContent(xml_buf)); + } + + if (ret != 0) { + vshError(ctl, _("Failed to detach interface")); ret = FALSE; - else { + } else { vshPrint(ctl, "%s", _("Interface detached successfully\n")); ret = TRUE; } @@ -6642,6 +6684,7 @@ static const vshCmdOptDef opts_attach_disk[] = { {"subdriver", VSH_OT_STRING, 0, gettext_noop("subdriver of disk device")}, {"type", VSH_OT_STRING, 0, gettext_noop("target device type")}, {"mode", VSH_OT_STRING, 0, gettext_noop("mode of device reading and writing")}, + {"persistent", VSH_OT_BOOL, 0, gettext_noop("persist disk attachment")}, {NULL, 0, 0, NULL} }; @@ -6652,6 +6695,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) char *source, *target, *driver, *subdriver, *type, *mode; int isFile = 0, ret = FALSE; char *buf = NULL, *tmp = NULL; + unsigned int flags; if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) goto cleanup; @@ -6767,12 +6811,22 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) if (!buf) goto cleanup; strcat(buf, " </disk>\n"); - if (virDomainAttachDevice(dom, buf)) - goto cleanup; - else - vshPrint(ctl, "%s", _("Disk attached successfully\n")); + if (vshCommandOptBool(cmd, "persistent")) { + flags = VIR_DOMAIN_DEVICE_MODIFY_CONFIG; + if (virDomainIsActive(dom) == 1) + flags |= VIR_DOMAIN_DEVICE_MODIFY_LIVE; + ret = virDomainAttachDeviceFlags(dom, buf, flags); + } else { + ret = virDomainAttachDevice(dom, buf); + } - ret = TRUE; + if (ret != 0) { + vshError(ctl, _("Failed to attach disk")); + ret = FALSE; + } else { + vshPrint(ctl, "%s", _("Disk attached successfully\n")); + ret = TRUE; + } cleanup: if (dom) @@ -6794,6 +6848,7 @@ static const vshCmdInfo info_detach_disk[] = { static const vshCmdOptDef opts_detach_disk[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("domain name, id or uuid")}, {"target", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("target of disk device")}, + {"persistent", VSH_OT_BOOL, 0, gettext_noop("persist disk detachment")}, {NULL, 0, 0, NULL} }; @@ -6809,6 +6864,7 @@ cmdDetachDisk(vshControl *ctl, const vshCmd *cmd) virDomainPtr dom = NULL; char *doc, *target; int i = 0, diff_tgt, ret = FALSE; + unsigned int flags; if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) goto cleanup; @@ -6874,10 +6930,21 @@ cmdDetachDisk(vshControl *ctl, const vshCmd *cmd) goto cleanup; } - ret = virDomainDetachDevice(dom, (char *)xmlBufferContent(xml_buf)); - if (ret != 0) + if (vshCommandOptBool(cmd, "persistent")) { + flags = VIR_DOMAIN_DEVICE_MODIFY_CONFIG; + if (virDomainIsActive(dom) == 1) + flags |= VIR_DOMAIN_DEVICE_MODIFY_LIVE; + ret = virDomainDetachDeviceFlags(dom, + (char *)xmlBufferContent(xml_buf), + flags); + } else { + ret = virDomainDetachDevice(dom, (char *)xmlBufferContent(xml_buf)); + } + + if (ret != 0) { + vshError(ctl, _("Failed to detach disk")); ret = FALSE; - else { + } else { vshPrint(ctl, "%s", _("Disk detached successfully\n")); ret = TRUE; }