[libvirt] [PATCH 0/2] Expose flags for several commands
APIs virDomainAttachDeviceFlags, virDomainDetachDeviceFlags, and virDomainUpdateDeviceFlags support three flags: VIR_DOMAIN_AFFECT_LIVE, VIR_DOMAIN_AFFECT_CONFIG, and VIR_DOMAIN_AFFECT_CURRENT, however, the commands based on those 3 APIs only expose one option "--persistent". These two patches expose the other two options as "--live" and "--current", and changes "--persistent" into "--config", just as other similiar commands do, e.g. "vcpupin", "schedinfo". Affected commands: attach-device attach-disk attach-interface detach-device detach-disk detach-interface update-device [PATCH 1/2] virsh: Add options for several commands which change domain config [PATCH 2/2] doc: Update docs for the changed commands Regards Osier
All of the following 7 commands just provide one option (--persistent) for user to specify how to affect the domain: attach-device detach-device attach-disk detach-disk attach-interface detach-interface update-device However, All of the APIs the above 7 commands use: virDomainAttachDeviceFlags, virDomainDetachDeviceFlags, and virDomainUpdateDeviceFlags support following 3 flags: VIR_DOMAIN_AFFECT_CURRENT VIR_DOMAIN_AFFECT_CONFIG VIR_DOMAIN_AFFECT_LIVE This patch add two new options (--live, --current), and changes "--persistent" into "--config", just as other similar commands, e.g. "schedinfo", "vcpupin". And since the APIs are designed as: If no flag is specified, behaviour is different depending on hypervisor, so virsh shouldn't do things like: if (virDomainIsActive(dom) == 1) flags |= VIR_DOMAIN_AFFECT_LIVE; This patch removes codes like this, leave the determination for underly hypervisor driver. --- tools/virsh.c | 242 ++++++++++++++++++++++++++++++++++++++++++--------------- 1 files changed, 178 insertions(+), 64 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index abc4614..4cd2e1a 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -9738,7 +9738,9 @@ static const vshCmdInfo info_attach_device[] = { static const vshCmdOptDef opts_attach_device[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("XML file")}, - {"persistent", VSH_OT_BOOL, 0, N_("persist device attachment")}, + {"config", VSH_OT_BOOL, 0, N_("affect next boot")}, + {"live", VSH_OT_BOOL, 0, N_("affect running domain")}, + {"current", VSH_OT_BOOL, 0, N_("affect current domain")}, {NULL, 0, 0, NULL} }; @@ -9749,7 +9751,26 @@ cmdAttachDevice(vshControl *ctl, const vshCmd *cmd) const char *from = NULL; char *buffer; int ret; - unsigned int flags; + int flags = 0; + + int current = vshCommandOptBool(cmd, "current"); + int config = vshCommandOptBool(cmd, "config"); + int live = vshCommandOptBool(cmd, "live"); + + if (current) { + if (live || config) { + vshError(ctl, "%s", _("--current must be specified exclusively")); + return false; + } + flags = VIR_DOMAIN_AFFECT_CURRENT; + } else { + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; + if (!config && !live) + flags = -1; + } if (!vshConnectionUsability(ctl, ctl->conn)) return false; @@ -9768,14 +9789,10 @@ cmdAttachDevice(vshControl *ctl, const vshCmd *cmd) return false; } - if (vshCommandOptBool(cmd, "persistent")) { - flags = VIR_DOMAIN_AFFECT_CONFIG; - if (virDomainIsActive(dom) == 1) - flags |= VIR_DOMAIN_AFFECT_LIVE; - ret = virDomainAttachDeviceFlags(dom, buffer, flags); - } else { + if (flags == -1) ret = virDomainAttachDevice(dom, buffer); - } + else + ret = virDomainAttachDeviceFlags(dom, buffer, flags); VIR_FREE(buffer); if (ret < 0) { @@ -9803,7 +9820,9 @@ static const vshCmdInfo info_detach_device[] = { static const vshCmdOptDef opts_detach_device[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("XML file")}, - {"persistent", VSH_OT_BOOL, 0, N_("persist device detachment")}, + {"config", VSH_OT_BOOL, 0, N_("affect next boot")}, + {"live", VSH_OT_BOOL, 0, N_("affect running domain")}, + {"current", VSH_OT_BOOL, 0, N_("affect current domain")}, {NULL, 0, 0, NULL} }; @@ -9814,7 +9833,26 @@ cmdDetachDevice(vshControl *ctl, const vshCmd *cmd) const char *from = NULL; char *buffer; int ret; - unsigned int flags; + int flags = 0; + + int current = vshCommandOptBool(cmd, "current"); + int config = vshCommandOptBool(cmd, "config"); + int live = vshCommandOptBool(cmd, "live"); + + if (current) { + if (live || config) { + vshError(ctl, "%s", _("--current must be specified exclusively")); + return false; + } + flags = VIR_DOMAIN_AFFECT_CURRENT; + } else { + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; + if (!config && !live) + flags = -1; + } if (!vshConnectionUsability(ctl, ctl->conn)) return false; @@ -9833,14 +9871,10 @@ cmdDetachDevice(vshControl *ctl, const vshCmd *cmd) return false; } - if (vshCommandOptBool(cmd, "persistent")) { - flags = VIR_DOMAIN_AFFECT_CONFIG; - if (virDomainIsActive(dom) == 1) - flags |= VIR_DOMAIN_AFFECT_LIVE; - ret = virDomainDetachDeviceFlags(dom, buffer, flags); - } else { + if (flags == -1) ret = virDomainDetachDevice(dom, buffer); - } + else + ret = virDomainDetachDeviceFlags(dom, buffer, flags); VIR_FREE(buffer); if (ret < 0) { @@ -9868,8 +9902,10 @@ static const vshCmdInfo info_update_device[] = { static const vshCmdOptDef opts_update_device[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("XML file")}, - {"persistent", VSH_OT_BOOL, 0, N_("persist device update")}, {"force", VSH_OT_BOOL, 0, N_("force device update")}, + {"config", VSH_OT_BOOL, 0, N_("affect next boot")}, + {"live", VSH_OT_BOOL, 0, N_("affect running domain")}, + {"current", VSH_OT_BOOL, 0, N_("affect current domain")}, {NULL, 0, 0, NULL} }; @@ -9880,7 +9916,24 @@ cmdUpdateDevice(vshControl *ctl, const vshCmd *cmd) const char *from = NULL; char *buffer; int ret; - unsigned int flags; + unsigned int flags = 0; + + int current = vshCommandOptBool(cmd, "current"); + int config = vshCommandOptBool(cmd, "config"); + int live = vshCommandOptBool(cmd, "live"); + + if (current) { + if (live || config) { + vshError(ctl, "%s", _("--current must be specified exclusively")); + return false; + } + flags = VIR_DOMAIN_AFFECT_CURRENT; + } else { + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; + } if (!vshConnectionUsability(ctl, ctl->conn)) return false; @@ -9899,14 +9952,6 @@ cmdUpdateDevice(vshControl *ctl, const vshCmd *cmd) return false; } - if (vshCommandOptBool(cmd, "persistent")) { - flags = VIR_DOMAIN_AFFECT_CONFIG; - if (virDomainIsActive(dom) == 1) - flags |= VIR_DOMAIN_AFFECT_LIVE; - } else { - flags = VIR_DOMAIN_AFFECT_LIVE; - } - if (vshCommandOptBool(cmd, "force")) flags |= VIR_DOMAIN_DEVICE_MODIFY_FORCE; @@ -9943,7 +9988,9 @@ static const vshCmdOptDef opts_attach_interface[] = { {"mac", VSH_OT_DATA, 0, N_("MAC address")}, {"script", VSH_OT_DATA, 0, N_("script used to bridge network interface")}, {"model", VSH_OT_DATA, 0, N_("model type")}, - {"persistent", VSH_OT_BOOL, 0, N_("persist interface attachment")}, + {"config", VSH_OT_BOOL, 0, N_("affect next boot")}, + {"live", VSH_OT_BOOL, 0, N_("affect running domain")}, + {"current", VSH_OT_BOOL, 0, N_("affect current domain")}, {NULL, 0, 0, NULL} }; @@ -9952,13 +9999,32 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom = NULL; const char *mac = NULL, *target = NULL, *script = NULL, - *type = NULL, *source = NULL, *model = NULL; + *type = NULL, *source = NULL, *model = NULL; int typ; int ret; bool functionReturn = false; - unsigned int flags; virBuffer buf = VIR_BUFFER_INITIALIZER; char *xml; + int flags = 0; + + int current = vshCommandOptBool(cmd, "current"); + int config = vshCommandOptBool(cmd, "config"); + int live = vshCommandOptBool(cmd, "live"); + + if (current) { + if (live || config) { + vshError(ctl, "%s", _("--current must be specified exclusively")); + return false; + } + flags = VIR_DOMAIN_AFFECT_CURRENT; + } else { + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; + if (!config && !live) + flags = -1; + } if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; @@ -10015,15 +10081,10 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd) xml = virBufferContentAndReset(&buf); - if (vshCommandOptBool(cmd, "persistent")) { - flags = VIR_DOMAIN_AFFECT_CONFIG; - if (virDomainIsActive(dom) == 1) - flags |= VIR_DOMAIN_AFFECT_LIVE; - ret = virDomainAttachDeviceFlags(dom, xml, flags); - } else { + if (flags == -1) ret = virDomainAttachDevice(dom, xml); - } - + else + ret = virDomainAttachDeviceFlags(dom, xml, flags); VIR_FREE(xml); if (ret != 0) { @@ -10053,7 +10114,9 @@ static const vshCmdOptDef opts_detach_interface[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, {"type", VSH_OT_DATA, VSH_OFLAG_REQ, N_("network interface type")}, {"mac", VSH_OT_STRING, 0, N_("MAC address")}, - {"persistent", VSH_OT_BOOL, 0, N_("persist interface detachment")}, + {"config", VSH_OT_BOOL, 0, N_("affect next boot")}, + {"live", VSH_OT_BOOL, 0, N_("affect running domain")}, + {"current", VSH_OT_BOOL, 0, N_("affect current domain")}, {NULL, 0, 0, NULL} }; @@ -10072,7 +10135,26 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd) int i = 0, diff_mac; int ret; int functionReturn = false; - unsigned int flags; + int flags = 0; + + int current = vshCommandOptBool(cmd, "current"); + int config = vshCommandOptBool(cmd, "config"); + int live = vshCommandOptBool(cmd, "live"); + + if (current) { + if (live || config) { + vshError(ctl, "%s", _("--current must be specified exclusively")); + return false; + } + flags = VIR_DOMAIN_AFFECT_CURRENT; + } else { + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; + if (!config && !live) + flags = -1; + } if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; @@ -10154,15 +10236,12 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd) goto cleanup; } - if (vshCommandOptBool(cmd, "persistent")) { - flags = VIR_DOMAIN_AFFECT_CONFIG; - if (virDomainIsActive(dom) == 1) - flags |= VIR_DOMAIN_AFFECT_LIVE; + if (flags == -1) { + ret = virDomainDetachDevice(dom, (char *)xmlBufferContent(xml_buf)); + } else { ret = virDomainDetachDeviceFlags(dom, (char *)xmlBufferContent(xml_buf), flags); - } else { - ret = virDomainDetachDevice(dom, (char *)xmlBufferContent(xml_buf)); } if (ret != 0) { @@ -10201,8 +10280,10 @@ static const vshCmdOptDef opts_attach_disk[] = { {"subdriver", VSH_OT_STRING, 0, N_("subdriver of disk device")}, {"type", VSH_OT_STRING, 0, N_("target device type")}, {"mode", VSH_OT_STRING, 0, N_("mode of device reading and writing")}, - {"persistent", VSH_OT_BOOL, 0, N_("persist disk attachment")}, {"sourcetype", VSH_OT_STRING, 0, N_("type of source (block|file)")}, + {"config", VSH_OT_BOOL, 0, N_("affect next boot")}, + {"live", VSH_OT_BOOL, 0, N_("affect running domain")}, + {"current", VSH_OT_BOOL, 0, N_("affect current domain")}, {NULL, 0, 0, NULL} }; @@ -10214,10 +10295,29 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) *subdriver = NULL, *type = NULL, *mode = NULL; bool isFile = false, functionReturn = false; int ret; - unsigned int flags; const char *stype = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; char *xml; + int flags = 0; + + int current = vshCommandOptBool(cmd, "current"); + int config = vshCommandOptBool(cmd, "config"); + int live = vshCommandOptBool(cmd, "live"); + + if (current) { + if (live || config) { + vshError(ctl, "%s", _("--current must be specified exclusively")); + return false; + } + flags = VIR_DOMAIN_AFFECT_CURRENT; + } else { + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; + if (!config && !live) + flags = -1; + } if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; @@ -10292,14 +10392,10 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) xml = virBufferContentAndReset(&buf); - if (vshCommandOptBool(cmd, "persistent")) { - flags = VIR_DOMAIN_AFFECT_CONFIG; - if (virDomainIsActive(dom) == 1) - flags |= VIR_DOMAIN_AFFECT_LIVE; - ret = virDomainAttachDeviceFlags(dom, xml, flags); - } else { + if (flags == -1) ret = virDomainAttachDevice(dom, xml); - } + else + ret = virDomainAttachDeviceFlags(dom, xml, flags); VIR_FREE(xml); @@ -10329,7 +10425,9 @@ static const vshCmdInfo info_detach_disk[] = { static const vshCmdOptDef opts_detach_disk[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, {"target", VSH_OT_DATA, VSH_OFLAG_REQ, N_("target of disk device")}, - {"persistent", VSH_OT_BOOL, 0, N_("persist disk detachment")}, + {"config", VSH_OT_BOOL, 0, N_("affect next boot")}, + {"live", VSH_OT_BOOL, 0, N_("affect running domain")}, + {"current", VSH_OT_BOOL, 0, N_("affect current domain")}, {NULL, 0, 0, NULL} }; @@ -10347,7 +10445,26 @@ cmdDetachDisk(vshControl *ctl, const vshCmd *cmd) int i = 0, diff_tgt; int ret; bool functionReturn = false; - unsigned int flags; + int flags = 0; + + int current = vshCommandOptBool(cmd, "current"); + int config = vshCommandOptBool(cmd, "config"); + int live = vshCommandOptBool(cmd, "live"); + + if (current) { + if (live || config) { + vshError(ctl, "%s", _("--current must be specified exclusively")); + return false; + } + flags = VIR_DOMAIN_AFFECT_CURRENT; + } else { + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; + if (!config && !live) + flags = -1; + } if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; @@ -10414,15 +10531,12 @@ cmdDetachDisk(vshControl *ctl, const vshCmd *cmd) goto cleanup; } - if (vshCommandOptBool(cmd, "persistent")) { - flags = VIR_DOMAIN_AFFECT_CONFIG; - if (virDomainIsActive(dom) == 1) - flags |= VIR_DOMAIN_AFFECT_LIVE; + if (flags == -1) { + ret = virDomainDetachDevice(dom, (char *)xmlBufferContent(xml_buf)); + } else { ret = virDomainDetachDeviceFlags(dom, (char *)xmlBufferContent(xml_buf), flags); - } else { - ret = virDomainDetachDevice(dom, (char *)xmlBufferContent(xml_buf)); } if (ret != 0) { -- 1.7.4
On 21.06.2011 07:37, Osier Yang wrote:
All of the following 7 commands just provide one option (--persistent) for user to specify how to affect the domain: attach-device detach-device attach-disk detach-disk attach-interface detach-interface update-device
However, All of the APIs the above 7 commands use: virDomainAttachDeviceFlags, virDomainDetachDeviceFlags, and virDomainUpdateDeviceFlags support following 3 flags: VIR_DOMAIN_AFFECT_CURRENT VIR_DOMAIN_AFFECT_CONFIG VIR_DOMAIN_AFFECT_LIVE
This patch add two new options (--live, --current), and changes "--persistent" into "--config", just as other similar commands, I am not fully convinced about this. I mean - it would be nice to have unified options names for these commands, but I am afraid we can't change them. e.g. "schedinfo", "vcpupin".
And since the APIs are designed as: If no flag is specified, behaviour is different depending on hypervisor, so virsh shouldn't do things like:
if (virDomainIsActive(dom) == 1) flags |= VIR_DOMAIN_AFFECT_LIVE;
This patch removes codes like this, leave the determination for underly hypervisor driver. --- tools/virsh.c | 242 ++++++++++++++++++++++++++++++++++++++++++--------------- 1 files changed, 178 insertions(+), 64 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index abc4614..4cd2e1a 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -9738,7 +9738,9 @@ static const vshCmdInfo info_attach_device[] = { static const vshCmdOptDef opts_attach_device[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("XML file")}, - {"persistent", VSH_OT_BOOL, 0, N_("persist device attachment")}, + {"config", VSH_OT_BOOL, 0, N_("affect next boot")}, + {"live", VSH_OT_BOOL, 0, N_("affect running domain")}, + {"current", VSH_OT_BOOL, 0, N_("affect current domain")}, Maybe it's a bit early morning for me, but I didn't get this description. We are always affecting current domain :) I would append 'status' word, resulting in "affect current domain status". This makes it clear for me. And maybe change description for 'vcpupin' command as well. But that could be a follow up patch.
Otherwise looking good. Michal
On 06/21/2011 04:55 PM, Michal Privoznik wrote:
On 21.06.2011 07:37, Osier Yang wrote:
All of the following 7 commands just provide one option (--persistent) for user to specify how to affect the domain: attach-device detach-device attach-disk detach-disk attach-interface detach-interface update-device
However, All of the APIs the above 7 commands use: virDomainAttachDeviceFlags, virDomainDetachDeviceFlags, and virDomainUpdateDeviceFlags support following 3 flags: VIR_DOMAIN_AFFECT_CURRENT VIR_DOMAIN_AFFECT_CONFIG VIR_DOMAIN_AFFECT_LIVE
This patch add two new options (--live, --current), and changes "--persistent" into "--config", just as other similar commands,
I am not fully convinced about this. I mean - it would be nice to have unified options names for these commands, but I am afraid we can't change them.
That's what I'm worried about too, changing "--persistent" into "--config" may affect already existed scripts based on virsh. But considering the unification, it might be better to change it. Or perhaps we can just leave "--persistent" unchanged, but it might be quite confused for user.
e.g. "schedinfo", "vcpupin".
And since the APIs are designed as: If no flag is specified, behaviour is different depending on hypervisor, so virsh shouldn't do things like:
if (virDomainIsActive(dom) == 1) flags |= VIR_DOMAIN_AFFECT_LIVE;
This patch removes codes like this, leave the determination for underly hypervisor driver. --- tools/virsh.c | 242 ++++++++++++++++++++++++++++++++++++++++++--------------- 1 files changed, 178 insertions(+), 64 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index abc4614..4cd2e1a 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -9738,7 +9738,9 @@ static const vshCmdInfo info_attach_device[] = { static const vshCmdOptDef opts_attach_device[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("XML file")}, - {"persistent", VSH_OT_BOOL, 0, N_("persist device attachment")}, + {"config", VSH_OT_BOOL, 0, N_("affect next boot")}, + {"live", VSH_OT_BOOL, 0, N_("affect running domain")}, + {"current", VSH_OT_BOOL, 0, N_("affect current domain")}, Maybe it's a bit early morning for me, but I didn't get this description. We are always affecting current domain :) I would append 'status' word, resulting in "affect current domain status". This makes it clear for me. And maybe change description for 'vcpupin' command as well. But that could be a follow up patch.
These descriptions used widely in the codes, agree it's a bit confused.
Otherwise looking good.
Michal
On 06/21/2011 03:07 AM, Osier Yang wrote:
This patch add two new options (--live, --current),
Good.
and changes "--persistent" into "--config", just as other similar commands,
NACK. Consistency may be nice, but backwards compatibility is more important. If we were to create an aliasing mechanism, where --persistent is still recognized as an alias for --config but not explicitly documented in the help output (but still mentioned in virsh.pod), then we could use that; in fact, we could add: VSH_OFLAG_ALIAS - if set, then the help field is the primary spelling for which this option spelling is an alias {"config", VSH_OT_BOOL, 0, N_("affect next boot")}, {"persistent", VSH_OT_BOOL, VSH_OT_ALIAS, "config"}, where only --config is documented in 'virsh help attach-device', but where 'virsh attach-device --persistent' behaves the same. But that's a much bigger prerequisite patch; the rest of your patch (adding --live and --current) is good whether or not we figure out a way to backwards-compatibly rename --persistent into --config.
This patch removes codes like this, leave the determination for underly
s/underly/underlying/
hypervisor driver. --- tools/virsh.c | 242 ++++++++++++++++++++++++++++++++++++++++++--------------- 1 files changed, 178 insertions(+), 64 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index abc4614..4cd2e1a 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -9738,7 +9738,9 @@ static const vshCmdInfo info_attach_device[] = { static const vshCmdOptDef opts_attach_device[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("XML file")}, - {"persistent", VSH_OT_BOOL, 0, N_("persist device attachment")}, + {"config", VSH_OT_BOOL, 0, N_("affect next boot")}, + {"live", VSH_OT_BOOL, 0, N_("affect running domain")}, + {"current", VSH_OT_BOOL, 0, N_("affect current domain")}, Maybe it's a bit early morning for me, but I didn't get this description. We are always affecting current domain :)
How about a separate cleanup patch which unifies all of these similar strings into: "config/persistent",... N_("alter persistent configuration, effect observed on next boot, error for transient domain") "live",... N_("alter running domain, immediate effect, error for transient domain") "current",... N_("either --config or --live according to current domain state") or is that too long? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
于 2011年06月22日 05:37, Eric Blake 写道:
On 06/21/2011 03:07 AM, Osier Yang wrote:
This patch add two new options (--live, --current),
Good.
and changes "--persistent" into "--config", just as other similar commands,
NACK. Consistency may be nice, but backwards compatibility is more important. If we were to create an aliasing mechanism, where --persistent is still recognized as an alias for --config but not explicitly documented in the help output (but still mentioned in virsh.pod), then we could use that; in fact, we could add:
VSH_OFLAG_ALIAS - if set, then the help field is the primary spelling for which this option spelling is an alias
{"config", VSH_OT_BOOL, 0, N_("affect next boot")}, {"persistent", VSH_OT_BOOL, VSH_OT_ALIAS, "config"},
where only --config is documented in 'virsh help attach-device', but where 'virsh attach-device --persistent' behaves the same. But that's a much bigger prerequisite patch; the rest of your patch (adding --live and --current) is good whether or not we figure out a way to backwards-compatibly rename --persistent into --config.
Sounds good idea, :-), will try this.
This patch removes codes like this, leave the determination for underly
s/underly/underlying/
OK.
hypervisor driver. --- tools/virsh.c | 242 ++++++++++++++++++++++++++++++++++++++++++--------------- 1 files changed, 178 insertions(+), 64 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index abc4614..4cd2e1a 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -9738,7 +9738,9 @@ static const vshCmdInfo info_attach_device[] = { static const vshCmdOptDef opts_attach_device[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("XML file")}, - {"persistent", VSH_OT_BOOL, 0, N_("persist device attachment")}, + {"config", VSH_OT_BOOL, 0, N_("affect next boot")}, + {"live", VSH_OT_BOOL, 0, N_("affect running domain")}, + {"current", VSH_OT_BOOL, 0, N_("affect current domain")}, Maybe it's a bit early morning for me, but I didn't get this description. We are always affecting current domain :)
How about a separate cleanup patch which unifies all of these similar strings into:
"config/persistent",... N_("alter persistent configuration, effect observed on next boot, error for transient domain") "live",... N_("alter running domain, immediate effect, error for transient domain") "current",... N_("either --config or --live according to current domain state")
This can be both of "--live" and "--config" for an active domain, depends on how underlying hypervisor driver implements it.
or is that too long?
Regards Osier
On Tue, Jun 21, 2011 at 05:07:43PM +0800, Osier Yang wrote:
On 06/21/2011 04:55 PM, Michal Privoznik wrote:
On 21.06.2011 07:37, Osier Yang wrote:
All of the following 7 commands just provide one option (--persistent) for user to specify how to affect the domain: attach-device detach-device attach-disk detach-disk attach-interface detach-interface update-device
This patch add two new options (--live, --current), and changes "--persistent" into "--config", just as other similar commands,
I am not fully convinced about this. I mean - it would be nice to have unified options names for these commands, but I am afraid we can't change them.
That's what I'm worried about too, changing "--persistent" into "--config" may affect already existed scripts based on virsh. But considering the unification, it might be better to change it.
Or perhaps we can just leave "--persistent" unchanged, but it might be quite confused for user.
We cannot remove an old flag, so --persistent has to stay. Now we can add an equivalent --config, which does the same for unification between commands, but removing --persistent will get a NACK from me :-) Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Mainly documents the new options (--live, --current, --config). Affected commands are: attach-device attach-disk attach-interface detach-device detach-disk detach-interface update-device --- tools/virsh.pod | 60 +++++++++++++++++++++++++++++++++++++++++------------- 1 files changed, 45 insertions(+), 15 deletions(-) diff --git a/tools/virsh.pod b/tools/virsh.pod index e7dcd41..1fbc27c 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -873,7 +873,8 @@ the single existing device; consider using B<update-device> for this usage. =item B<attach-disk> I<domain-id> I<source> I<target> optional I<--driver driver> I<--subdriver subdriver> I<--type type> -I<--mode mode> I<--persistent> I<--sourcetype soucetype> +I<--mode mode> I<--sourcetype soucetype> I<--live> I<--config> +I<--current> Attach a new disk device to the domain. I<source> and I<target> are paths for the files and devices. @@ -882,12 +883,16 @@ I<type> can indicate I<cdrom> or I<floppy> as alternative to the disk default, although this use only replaces the media within the existing virtual cdrom or floppy device; consider using B<update-device> for this usage instead. I<mode> can specify the two specific mode I<readonly> or I<shareable>. -I<persistent> indicates the changes will affect the next boot of the domain. I<sourcetype> can indicate the type of source (block|file) +If I<--live> is specified, affect a running guest. +If I<--config> is specified, affect the next boot of a persistent guest. +If I<--current> is specified, affect the current guest state. +Both I<--live> and I<--config> flags may be given, but I<--current> is exclusive. +If no flag is specified, behavior is different depending on hypervisor. =item B<attach-interface> I<domain-id> I<type> I<source> optional I<--target target> I<--mac mac> I<--script script> I<--model model> -I<--persistent> +I<--live> I<--config> I<--current> Attach a new network interface to the domain. I<type> can be either I<network> to indicate a physical network device or I<bridge> to indicate a bridge to a device. @@ -897,33 +902,58 @@ I<mac> allows to specify the MAC address of the network interface. I<script> allows to specify a path to a script handling a bridge instead of the default one. I<model> allows to specify the model type. -I<persistent> indicates the changes will affect the next boot of the domain. +If I<--live> is specified, affect a running guest. +If I<--config> is specified, affect the next boot of a persistent guest. +If I<--current> is specified, affect the current guest state. +Both I<--live> and I<--config> flags may be given, but I<--current> is exclusive. +If no flag is specified, behavior is different depending on hypervisor. -=item B<detach-device> I<domain-id> I<FILE> +=item B<detach-device> I<domain-id> I<FILE> I<--live> I<--config> I<--current> Detach a device from the domain, takes the same kind of XML descriptions as command B<attach-device>. +If I<--live> is specified, affect a running guest. +If I<--config> is specified, affect the next boot of a persistent guest. +If I<--current> is specified, affect the current guest state. +Both I<--live> and I<--config> flags may be given, but I<--current> is exclusive. +If no flag is specified, behavior is different depending on hypervisor. -=item B<detach-disk> I<domain-id> I<target> +=item B<detach-disk> I<domain-id> I<target> I<--live> I<--config> I<--current> Detach a disk device from a domain. The I<target> is the device as seen from the domain. +If I<--live> is specified, affect a running guest. +If I<--config> is specified, affect the next boot of a persistent guest. +If I<--current> is specified, affect the current guest state. +Both I<--live> and I<--config> flags may be given, but I<--current> is exclusive. +If no flag is specified, behavior is different depending on hypervisor. =item B<detach-interface> I<domain-id> I<type> optional I<--mac mac> +I<--live> I<--config> I<--current> Detach a network interface from a domain. -I<type> can be either I<network> to indicate a physical network device or I<bridge> to indicate a bridge to a device. -It is recommended to use the I<mac> option to distinguish between the interfaces -if more than one are present on the domain. +I<type> can be either I<network> to indicate a physical network device or +I<bridge> to indicate a bridge to a device. It is recommended to use the +I<mac> option to distinguish between the interfaces if more than one are +present on the domain. +If I<--live> is specified, affect a running guest. +If I<--config> is specified, affect the next boot of a persistent guest. +If I<--current> is specified, affect the current guest state. +Both I<--live> and I<--config> flags may be given, but I<--current> is exclusive. +If no flag is specified, behavior is different depending on hypervisor. -=item B<update-device> I<domain-id> I<file> optional I<--persistent> I<--force> +=item B<update-device> I<domain-id> I<file> optional I<--force> I<--live> +I<--config> I<--current> Update the characteristics of a device associated with I<domain-id>, based on -the device definition in an XML I<file>. If the I<--persistent> option is -used, the changes will affect the next boot of the domain. The I<--force> -option can be used to force device update, e.g., to eject a CD-ROM even if it -is locked/mounted in the domain. See the documentation to learn about libvirt -XML format for a device. +the device definition in an XML I<file>. The I<--force> option can be used to +force device update, e.g., to eject a CD-ROM even if it is locked/mounted in +the domain. See the documentation to learn about libvirt XML format for a device. +If I<--live> is specified, affect a running guest. +If I<--config> is specified, affect the next boot of a persistent guest. +If I<--current> is specified, affect the current guest state. +Both I<--live> and I<--config> flags may be given, but I<--current> is exclusive. +If no flag is specified, behavior is different depending on hypervisor. =back -- 1.7.4
participants (4)
-
Daniel Veillard -
Eric Blake -
Michal Privoznik -
Osier Yang