[libvirt] [PATCH 0/5] Add domain modification impact flags to hot-management commands
As promised in the RFC, this series adds the --live, --config, --current and --persistent flags to virsh commands that were lacking them. Peter Krempa (5): virsh-domain: Fix declarations of flag variables in cmdChangeMedia virsh: Fix semantics of --config for "update-device" command virsh-domain: Add --live, --config, --current logic to cmdDetachInterface virsh-domain: Add --live, --config, --current logic to cmdDetachDevice virsh-domain: Add --live, --config, --current logic to cmdDetachDisk tools/virsh-domain.c | 225 ++++++++++++++++++++++++++++++++++++--------------- tools/virsh.pod | 66 ++++++++++----- 2 files changed, 206 insertions(+), 85 deletions(-) -- 1.8.1.5
"flags" were declared as signed and the parameter options can be declared directly. Also use macros for mutual exclusion on some of the incompatible parameter variables. --- tools/virsh-domain.c | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 11db36b..68df01e 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -9903,26 +9903,21 @@ cmdChangeMedia(vshControl *ctl, const vshCmd *cmd) const char *doc = NULL; xmlNodePtr disk_node = NULL; const char *disk_xml = NULL; - unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; - bool config, live, current, force = false; - bool eject, insert, update = false; bool ret = false; int prepare_type = 0; const char *action = NULL; + bool config = vshCommandOptBool(cmd, "config"); + bool live = vshCommandOptBool(cmd, "live"); + bool current = vshCommandOptBool(cmd, "current"); + bool force = vshCommandOptBool(cmd, "force"); + bool eject = vshCommandOptBool(cmd, "eject"); + bool insert = vshCommandOptBool(cmd, "insert"); + bool update = vshCommandOptBool(cmd, "update"); + unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; - config = vshCommandOptBool(cmd, "config"); - live = vshCommandOptBool(cmd, "live"); - current = vshCommandOptBool(cmd, "current"); - force = vshCommandOptBool(cmd, "force"); - eject = vshCommandOptBool(cmd, "eject"); - insert = vshCommandOptBool(cmd, "insert"); - update = vshCommandOptBool(cmd, "update"); - - if (eject + insert + update > 1) { - vshError(ctl, "%s", _("--eject, --insert, and --update must be specified " - "exclusively.")); - return false; - } + VSH_EXCLUSIVE_OPTIONS_VAR(eject, insert); + VSH_EXCLUSIVE_OPTIONS_VAR(eject, update); + VSH_EXCLUSIVE_OPTIONS_VAR(insert, update); if (eject) { prepare_type = VSH_PREPARE_DISK_XML_EJECT; -- 1.8.1.5
On 03/21/2013 05:42 PM, Peter Krempa wrote:
"flags" were declared as signed and the parameter options can be declared directly.
I don't see any 'signed flags' here, ACK after 1.0.4 with that sentece removed. Martin
The man page states that with --config the next boot is affected. This can be understood as if _only_ the next bood was affected. This isn't true if the machine is running. This patch adds the full --live, --config, --current infrastructure and tweaks stuff to correctly support the obsolete --persistent flag. --- tools/virsh-domain.c | 41 ++++++++++++++++++++++++++++++----------- tools/virsh.pod | 21 ++++++++++++++------- 2 files changed, 44 insertions(+), 18 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 68df01e..6741837 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -9246,13 +9246,21 @@ static const vshCmdOptDef opts_update_device[] = { .help = N_("XML file") }, {.name = "persistent", - .type = VSH_OT_ALIAS, - .help = "config" + .type = VSH_OT_BOOL, + .help = N_("make live change persistent") }, {.name = "config", .type = VSH_OT_BOOL, .help = N_("affect next boot") }, + {.name = "live", + .type = VSH_OT_BOOL, + .help = N_("affect running domain") + }, + {.name = "current", + .type = VSH_OT_BOOL, + .help = N_("affect current domain") + }, {.name = "force", .type = VSH_OT_BOOL, .help = N_("force device update") @@ -9267,7 +9275,22 @@ cmdUpdateDevice(vshControl *ctl, const vshCmd *cmd) const char *from = NULL; char *buffer = NULL; bool ret = false; - unsigned int flags; + unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; + bool current = vshCommandOptBool(cmd, "current"); + bool config = vshCommandOptBool(cmd, "config"); + bool live = vshCommandOptBool(cmd, "live"); + bool persistent = vshCommandOptBool(cmd, "persistent"); + + VSH_EXCLUSIVE_OPTIONS_VAR(persistent, live); + VSH_EXCLUSIVE_OPTIONS_VAR(persistent, current); + + VSH_EXCLUSIVE_OPTIONS_VAR(current, live); + VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + + if (config || persistent) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false; @@ -9275,19 +9298,15 @@ cmdUpdateDevice(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptStringReq(ctl, cmd, "file", &from) < 0) goto cleanup; + if (persistent && + virDomainIsActive(dom) == 1) + flags |= VIR_DOMAIN_AFFECT_LIVE; + if (virFileReadAll(from, VSH_MAX_XML_FILE, &buffer) < 0) { vshReportError(ctl); goto cleanup; } - if (vshCommandOptBool(cmd, "config")) { - 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; diff --git a/tools/virsh.pod b/tools/virsh.pod index b5e632e..a9e8c65 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1887,18 +1887,25 @@ If I<--config> is specified, alter persistent configuration, effect observed on next boot, for compatibility purposes, I<--persistent> is alias of I<--config>. -=item B<update-device> I<domain> I<file> [I<--config>] [I<--force>] +=item B<update-device> I<domain> I<file> [I<--force>] +[[[I<--live>] [I<--config>] | [I<--current>]] | [I<--persistent>]] Update the characteristics of a device associated with I<domain>, -based on the device definition in an XML I<file>. If the I<--config> -option is used, the changes will take affect the next time libvirt -starts the domain. For compatibility purposes, I<--persistent> is -alias of I<--config>. 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 at +based on 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 at L<http://libvirt.org/formatdomain.html#elementsDevices> to learn about libvirt XML format for a device. +If I<--live> is specified, affect a running domain. +If I<--config> is specified, affect the next startup of a persistent domain. +If I<--current> is specified, affect the current domain state. +Both I<--live> and I<--config> flags may be given, but I<--current> is +exclusive. Not specifying any flag is the same as specifying I<--current>. + +For compatibility purposes, I<--persistent> behaves like I<--config> for +an offline domain, and like I<--live> I<--config> for a running domain. + =item B<change-media> I<domain> I<path> [I<--eject>] [I<--insert>] [I<--update>] [I<source>] [I<--force>] [[I<--live>] [I<--config>] | [I<--current>]] -- 1.8.1.5
On 03/21/2013 05:42 PM, Peter Krempa wrote:
The man page states that with --config the next boot is affected. This can be understood as if _only_ the next bood was affected. This isn't
s/bood/boot/
true if the machine is running.
This patch adds the full --live, --config, --current infrastructure and tweaks stuff to correctly support the obsolete --persistent flag. --- tools/virsh-domain.c | 41 ++++++++++++++++++++++++++++++----------- tools/virsh.pod | 21 ++++++++++++++------- 2 files changed, 44 insertions(+), 18 deletions(-)
[...]
@@ -9267,7 +9275,22 @@ cmdUpdateDevice(vshControl *ctl, const vshCmd *cmd) const char *from = NULL; char *buffer = NULL; bool ret = false; - unsigned int flags; + unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; + bool current = vshCommandOptBool(cmd, "current"); + bool config = vshCommandOptBool(cmd, "config"); + bool live = vshCommandOptBool(cmd, "live"); + bool persistent = vshCommandOptBool(cmd, "persistent"); + + VSH_EXCLUSIVE_OPTIONS_VAR(persistent, live);
Previously, --persistent --live was working and made sense as well, but you are disallowing that now. With that in mind, why do you allow --persistent --config then?
From my POV, I'd leave --persistent --live --config allowed. The change in what --persistent does (affects also running domain, but it didn't before), is OK with me.
ACK after 1.0.4 with that one line removed. Martin
On 03/21/2013 12:42 PM, Peter Krempa wrote:
The man page states that with --config the next boot is affected. This can be understood as if _only_ the next bood was affected. This isn't true if the machine is running.
Are you certain of that? My understanding was that when --config was specified, *only* the persistent config should be changed, but not the live state, regardless of whether or not the domain is running. Otherwise, there is no way to change just the persistent config of a running domain. Or is your explanation incorrect, and the code correct? Here is what I *thought* was the meaning of these options: --config - only change the persistent config, but not the live state. The command should fail for a transient domain. --live - only change the live state, but not the persistent config. The command should fail for a domain that isn't running. --current - useless (really, I mean that) because its meaning is different depending on whether or not the domain is running --persistent - deprecated synonym for --config no option - also useless because it means the same thing as --current What's missing: a way to say "change both live state and persistent config as appropriate" After discussing it on the list, in the name of consistency I actually very reluctantly implemented this same logic for virsh net-update even though I thought it was terrible. Did I get it wrong? So what is the *real* meaning of each of these options? (and are you sure you're not changing the meaning of any of them?)
On 03/27/13 16:22, Laine Stump wrote:
On 03/21/2013 12:42 PM, Peter Krempa wrote:
The man page states that with --config the next boot is affected. This can be understood as if _only_ the next bood was affected. This isn't true if the machine is running.
Are you certain of that? My understanding was that when --config was specified, *only* the persistent config should be changed, but not the live state, regardless of whether or not the domain is running. Otherwise, there is no way to change just the persistent config of a running domain.
Yes, that's why I'm doing this. Before this patch --config meant "modify the persistent config along with the live change" and there was no way to use this virsh command to change the next-boot config if the domain was running.
Or is your explanation incorrect, and the code correct?
Here is what I *thought* was the meaning of these options:
--config - only change the persistent config, but not the live state. The command should fail for a transient domain.
--live - only change the live state, but not the persistent config. The command should fail for a domain that isn't running.
--current - useless (really, I mean that) because its meaning is different depending on whether or not the domain is running
Yes those we understand in the same way.
--persistent - deprecated synonym for --config
This used to be the synonym for config, but this might be undestood as the option you are missing later on ...
no option - also useless because it means the same thing as --current
What's missing: a way to say "change both live state and persistent config as appropriate"
Well, this patch would abuse the --persistent flag for this purpose. It adds _CONFIG always and _LIVE in case the domain is up.
After discussing it on the list, in the name of consistency I actually very reluctantly implemented this same logic for virsh net-update even though I thought it was terrible. Did I get it wrong?
No, you've got it correct. Well, except for --persistent but I'm less sure about that than you.
So what is the *real* meaning of each of these options? (and are you sure you're not changing the meaning of any of them?)
Except for --persistent, the meaning seems to be well defined now and used appropriately in new places. The problem is with the legacy API's that didn't take up on this approach yet.
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 03/27/2013 11:39 AM, Peter Krempa wrote:
On 03/27/13 16:22, Laine Stump wrote:
On 03/21/2013 12:42 PM, Peter Krempa wrote:
The man page states that with --config the next boot is affected. This can be understood as if _only_ the next bood was affected. This isn't true if the machine is running.
Are you certain of that? My understanding was that when --config was specified, *only* the persistent config should be changed, but not the live state, regardless of whether or not the domain is running. Otherwise, there is no way to change just the persistent config of a running domain.
Yes, that's why I'm doing this. Before this patch --config meant "modify the persistent config along with the live change" and there was no way to use this virsh command to change the next-boot config if the domain was running.
Or is your explanation incorrect, and the code correct?
Here is what I *thought* was the meaning of these options:
--config - only change the persistent config, but not the live state. The command should fail for a transient domain.
--live - only change the live state, but not the persistent config. The command should fail for a domain that isn't running.
--current - useless (really, I mean that) because its meaning is different depending on whether or not the domain is running
Yes those we understand in the same way.
--persistent - deprecated synonym for --config
This used to be the synonym for config, but this might be undestood as the option you are missing later on ...
no option - also useless because it means the same thing as --current
What's missing: a way to say "change both live state and persistent config as appropriate"
Well, this patch would abuse the --persistent flag for this purpose. It adds _CONFIG always and _LIVE in case the domain is up.
After discussing it on the list, in the name of consistency I actually very reluctantly implemented this same logic for virsh net-update even though I thought it was terrible. Did I get it wrong?
No, you've got it correct. Well, except for --persistent but I'm less sure about that than you.
So what is the *real* meaning of each of these options? (and are you sure you're not changing the meaning of any of them?)
Except for --persistent, the meaning seems to be well defined now and used appropriately in new places. The problem is with the legacy API's that didn't take up on this approach yet.
I'm concerned about changing the meaning of any existing option, which creates trouble for any existing script that uses the options. An example of the effects of this can be seen in this thread: https://www.redhat.com/archives/libvirt-users/2013-March/msg00108.html In short, netfilter changed the meaning of the iptables commandline --ctdir option which, although it was incorrect, had already been in release versions for 2 years. By changing this option, with no way to detect presence/absence of the change short of this very heroic effort by stefanb: https://www.redhat.com/archives/libvir-list/2013-March/msg01403.html they effectively made it unusable except in cases of packages that are closely tied to a specific version of kernel/netfilter. Although it's not listed in a .h file anywhere, commandline options *are* part of a public API, and shouldn't be changed after they've been in an official release (unless they were broken to the point that nobody would have been able to use them anyway).
On 03/27/13 17:25, Laine Stump wrote:
On 03/27/2013 11:39 AM, Peter Krempa wrote:
On 03/27/13 16:22, Laine Stump wrote:
On 03/21/2013 12:42 PM, Peter Krempa wrote:
The man page states that with --config the next boot is affected. This can be understood as if _only_ the next bood was affected. This isn't true if the machine is running.
Are you certain of that? My understanding was that when --config was specified, *only* the persistent config should be changed, but not the live state, regardless of whether or not the domain is running. Otherwise, there is no way to change just the persistent config of a running domain.
Yes, that's why I'm doing this. Before this patch --config meant "modify the persistent config along with the live change" and there was no way to use this virsh command to change the next-boot config if the domain was running.
Or is your explanation incorrect, and the code correct?
Here is what I *thought* was the meaning of these options:
--config - only change the persistent config, but not the live state. The command should fail for a transient domain.
--live - only change the live state, but not the persistent config. The command should fail for a domain that isn't running.
--current - useless (really, I mean that) because its meaning is different depending on whether or not the domain is running
Yes those we understand in the same way.
--persistent - deprecated synonym for --config
This used to be the synonym for config, but this might be undestood as the option you are missing later on ...
no option - also useless because it means the same thing as --current
What's missing: a way to say "change both live state and persistent config as appropriate"
Well, this patch would abuse the --persistent flag for this purpose. It adds _CONFIG always and _LIVE in case the domain is up.
After discussing it on the list, in the name of consistency I actually very reluctantly implemented this same logic for virsh net-update even though I thought it was terrible. Did I get it wrong?
No, you've got it correct. Well, except for --persistent but I'm less sure about that than you.
So what is the *real* meaning of each of these options? (and are you sure you're not changing the meaning of any of them?)
Except for --persistent, the meaning seems to be well defined now and used appropriately in new places. The problem is with the legacy API's that didn't take up on this approach yet.
I'm concerned about changing the meaning of any existing option, which creates trouble for any existing script that uses the options. An example of the effects of this can be seen in this thread:
https://www.redhat.com/archives/libvirt-users/2013-March/msg00108.html
In short, netfilter changed the meaning of the iptables commandline --ctdir option which, although it was incorrect, had already been in release versions for 2 years. By changing this option, with no way to detect presence/absence of the change short of this very heroic effort by stefanb:
https://www.redhat.com/archives/libvir-list/2013-March/msg01403.html
they effectively made it unusable except in cases of packages that are closely tied to a specific version of kernel/netfilter.
Although it's not listed in a .h file anywhere, commandline options *are* part of a public API, and shouldn't be changed after they've been in an official release (unless they were broken to the point that nobody would have been able to use them anyway).
Looking through really old versions of virsh I found that the --persistent flag was used in the meaning "make this live change also affect next boot" afterwards we decided to shed --persistent for --config with a pure rename. this change introduced the discrepancy between the original use of --persistent and the new meaning of --config. Right now, I don't feel like introducing yet another set of domain modification scope flags and I'd rather fix all the improper uses of --config to be uniform and try to preserve the original meaning of the --persistent flag as it seems useful to me. Summary of the changed behavior: 1.) --persistent behaves like it has since it was introduced 2.) --persistent is documented again 3.) --config no longer influences the live guest (it isn't doing so in other places, this behavior is known from other places and (sort-of) documented) 4.) the user is able to force use of the new API that has better specified behavior as in the case of the api without flags. In case we decide against this slight change (on running domain --config won't longer influence the running state) my approach to fix the user confusion will be very different. I'll leave the code in place as-is and I will only document that the --config flag behaves differently on these API's compared to other places. The reason I started doing this is user confusion because of the different meaning and bad docs. I don't really care that much about the change, but it's always nice to have stuff done in a consistent way, so I proposed this. Peter
On 03/27/2013 10:49 AM, Peter Krempa wrote:
Here is what I *thought* was the meaning of these options:
--config - only change the persistent config, but not the live state. The command should fail for a transient domain.
--live - only change the live state, but not the persistent config. The command should fail for a domain that isn't running.
--current - useless (really, I mean that) because its meaning is different depending on whether or not the domain is running
Yes those we understand in the same way.
I agree. And these are the new flags, which most commands have (just a few of the older ones haven't added them yet).
--persistent - deprecated synonym for --config
Possibly,...
This used to be the synonym for config, but this might be undestood as the option you are missing later on ...
...but I think that it is a great name to use for sane semantics (--config always, and --live as well if the domain is running). I'm not sure how many old commands that exposed --persistent have anything other than the sane semantics, though.
no option - also useless because it means the same thing as --current
What's missing: a way to say "change both live state and persistent config as appropriate"
Well, this patch would abuse the --persistent flag for this purpose. It adds _CONFIG always and _LIVE in case the domain is up.
Indeed, that is the sanest semantics. I don't think we can make it default, unless maybe we add a command to set the default effect, make ALL virsh code that uses --config/--live call into the common routine to learn what default to use when there is no flag, and make a persistent .ini-style file that remembers the default setting you'd like virsh to have.
After discussing it on the list, in the name of consistency I actually very reluctantly implemented this same logic for virsh net-update even though I thought it was terrible. Did I get it wrong?
No, you've got it correct. Well, except for --persistent but I'm less sure about that than you.
So what is the *real* meaning of each of these options? (and are you sure you're not changing the meaning of any of them?)
Well, there _have_ been instances where the underlying qemu_driver.c has had broken semantics in some releases, where we fix it later (such as cases of --config not actually changing the config settings, or --config --live together only changing one instead of both), so maybe we argue that any use of --persistent with anything other than sane semantics is just a (long-standing) bug?
Except for --persistent, the meaning seems to be well defined now and used appropriately in new places. The problem is with the legacy API's that didn't take up on this approach yet.
I'm concerned about changing the meaning of any existing option, which creates trouble for any existing script that uses the options. An example of the effects of this can be seen in this thread:
https://www.redhat.com/archives/libvirt-users/2013-March/msg00108.html
Yeah, anywhere we have an old API that used --persistent, I'm a bit reluctant to accidentally change that flag, even if changing it to sane semantics makes the most sense. We're almost stuck with needing to invent yet another flag name, that we can use universally without having to worry about breaking back-compat.
Although it's not listed in a .h file anywhere, commandline options *are* part of a public API, and shouldn't be changed after they've been in an official release (unless they were broken to the point that nobody would have been able to use them anyway).
Well, being inconsistent with all other commands does sound kind of like being broken to the point that no one knows which ones to use.
Looking through really old versions of virsh I found that the --persistent flag was used in the meaning "make this live change also affect next boot" afterwards we decided to shed --persistent for --config with a pure rename. this change introduced the discrepancy between the original use of --persistent and the new meaning of --config.
Right now, I don't feel like introducing yet another set of domain modification scope flags and I'd rather fix all the improper uses of --config to be uniform and try to preserve the original meaning of the --persistent flag as it seems useful to me.
I tend to agree - the old semantics of --persistent really WERE sane, and we broke things when we tried to make --persistent an alias of --config.
Summary of the changed behavior: 1.) --persistent behaves like it has since it was introduced 2.) --persistent is documented again 3.) --config no longer influences the live guest (it isn't doing so in other places, this behavior is known from other places and (sort-of) documented) 4.) the user is able to force use of the new API that has better specified behavior as in the case of the api without flags.
Yes, I would be able to live with such a change.
In case we decide against this slight change (on running domain --config won't longer influence the running state) my approach to fix the user confusion will be very different. I'll leave the code in place as-is and I will only document that the --config flag behaves differently on these API's compared to other places.
This feels a bit worse; making --config a synonym for --persistent was probably the wrong way to go about it.
The reason I started doing this is user confusion because of the different meaning and bad docs.
I guess we can still document (for any option where meanings might have changed) that the user needs to be aware that older hypervisors might act incorrectly on a --config request. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Use the established approach to improve this function too. --- tools/virsh-domain.c | 52 +++++++++++++++++++++++++++++++++++++++------------- tools/virsh.pod | 15 +++++++++++---- 2 files changed, 50 insertions(+), 17 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 6741837..df72c78 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -9353,13 +9353,25 @@ static const vshCmdOptDef opts_detach_interface[] = { .help = N_("MAC address") }, {.name = "persistent", - .type = VSH_OT_ALIAS, - .help = "config" + .type = VSH_OT_BOOL, + .help = N_("make live change persistent") }, {.name = "config", .type = VSH_OT_BOOL, .help = N_("affect next boot") }, + {.name = "live", + .type = VSH_OT_BOOL, + .help = N_("affect running domain") + }, + {.name = "current", + .type = VSH_OT_BOOL, + .help = N_("affect current domain") + }, + {.name = "force", + .type = VSH_OT_BOOL, + .help = N_("force device update") + }, {.name = NULL} }; @@ -9373,12 +9385,27 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd) xmlNodePtr cur = NULL, matchNode = NULL; xmlBufferPtr xml_buf = NULL; const char *mac =NULL, *type = NULL; - char *doc; + char *doc = NULL; char buf[64]; int i = 0, diff_mac; int ret; int functionReturn = false; - unsigned int flags; + unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; + bool current = vshCommandOptBool(cmd, "current"); + bool config = vshCommandOptBool(cmd, "config"); + bool live = vshCommandOptBool(cmd, "live"); + bool persistent = vshCommandOptBool(cmd, "persistent"); + + VSH_EXCLUSIVE_OPTIONS_VAR(persistent, live); + VSH_EXCLUSIVE_OPTIONS_VAR(persistent, current); + + VSH_EXCLUSIVE_OPTIONS_VAR(current, live); + VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + + if (config || persistent) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false; @@ -9389,13 +9416,14 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptStringReq(ctl, cmd, "mac", &mac) < 0) goto cleanup; - doc = virDomainGetXMLDesc(dom, 0); - if (!doc) + if (persistent && + virDomainIsActive(dom) == 1) + flags |= VIR_DOMAIN_AFFECT_LIVE; + + if (!(doc = virDomainGetXMLDesc(dom, 0))) goto cleanup; - xml = virXMLParseStringCtxt(doc, _("(domain_definition)"), &ctxt); - VIR_FREE(doc); - if (!xml) { + if (!(xml = virXMLParseStringCtxt(doc, _("(domain_definition)"), &ctxt))) { vshError(ctl, "%s", _("Failed to get interface information")); goto cleanup; } @@ -9460,10 +9488,7 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd) goto cleanup; } - if (vshCommandOptBool(cmd, "config")) { - flags = VIR_DOMAIN_AFFECT_CONFIG; - if (virDomainIsActive(dom) == 1) - flags |= VIR_DOMAIN_AFFECT_LIVE; + if (flags != 0) { ret = virDomainDetachDeviceFlags(dom, (char *)xmlBufferContent(xml_buf), flags); @@ -9479,6 +9504,7 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd) } cleanup: + VIR_FREE(doc); virDomainFree(dom); xmlXPathFreeObject(obj); xmlXPathFreeContext(ctxt); diff --git a/tools/virsh.pod b/tools/virsh.pod index a9e8c65..ebbe201 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1876,16 +1876,23 @@ If I<--config> is specified, alter persistent configuration, effect observed on next boot, for compatibility purposes, I<--persistent> is alias of I<--config>. -=item B<detach-interface> I<domain> I<type> [I<--mac mac>] [I<--config>] +=item B<detach-interface> I<domain> I<type> [I<--mac mac>] +[[[I<--live>] [I<--config>] | [I<--current>]] | [I<--persistent>]] 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. -If I<--config> is specified, alter persistent configuration, effect observed -on next boot, for compatibility purposes, I<--persistent> is alias of -I<--config>. + +If I<--live> is specified, affect a running domain. +If I<--config> is specified, affect the next startup of a persistent domain. +If I<--current> is specified, affect the current domain state. +Both I<--live> and I<--config> flags may be given, but I<--current> is +exclusive. Not specifying any flag is the same as specifying I<--current>. + +For compatibility purposes, I<--persistent> behaves like I<--config> for +an offline domain, and like I<--live> I<--config> for a running domain. =item B<update-device> I<domain> I<file> [I<--force>] [[[I<--live>] [I<--config>] | [I<--current>]] | [I<--persistent>]] -- 1.8.1.5
On 03/21/2013 05:42 PM, Peter Krempa wrote:
Use the established approach to improve this function too. --- tools/virsh-domain.c | 52 +++++++++++++++++++++++++++++++++++++++------------- tools/virsh.pod | 15 +++++++++++---- 2 files changed, 50 insertions(+), 17 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 6741837..df72c78 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -9353,13 +9353,25 @@ static const vshCmdOptDef opts_detach_interface[] = { .help = N_("MAC address") }, {.name = "persistent", - .type = VSH_OT_ALIAS, - .help = "config" + .type = VSH_OT_BOOL, + .help = N_("make live change persistent") }, {.name = "config", .type = VSH_OT_BOOL, .help = N_("affect next boot") }, + {.name = "live", + .type = VSH_OT_BOOL, + .help = N_("affect running domain") + }, + {.name = "current", + .type = VSH_OT_BOOL, + .help = N_("affect current domain") + }, + {.name = "force", + .type = VSH_OT_BOOL, + .help = N_("force device update") + },
Adding non-used --force paramter, copy-paste error?
{.name = NULL} };
@@ -9373,12 +9385,27 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd) xmlNodePtr cur = NULL, matchNode = NULL; xmlBufferPtr xml_buf = NULL; const char *mac =NULL, *type = NULL; - char *doc; + char *doc = NULL; char buf[64]; int i = 0, diff_mac; int ret; int functionReturn = false; - unsigned int flags; + unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; + bool current = vshCommandOptBool(cmd, "current"); + bool config = vshCommandOptBool(cmd, "config"); + bool live = vshCommandOptBool(cmd, "live"); + bool persistent = vshCommandOptBool(cmd, "persistent"); + + VSH_EXCLUSIVE_OPTIONS_VAR(persistent, live);
Same as in previous patch, remove this line.
+ VSH_EXCLUSIVE_OPTIONS_VAR(persistent, current); + + VSH_EXCLUSIVE_OPTIONS_VAR(current, live); + VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + + if (config || persistent) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE;
if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false; @@ -9389,13 +9416,14 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptStringReq(ctl, cmd, "mac", &mac) < 0) goto cleanup;
- doc = virDomainGetXMLDesc(dom, 0); - if (!doc) + if (persistent && + virDomainIsActive(dom) == 1) + flags |= VIR_DOMAIN_AFFECT_LIVE; + + if (!(doc = virDomainGetXMLDesc(dom, 0))) goto cleanup;
- xml = virXMLParseStringCtxt(doc, _("(domain_definition)"), &ctxt); - VIR_FREE(doc); - if (!xml) { + if (!(xml = virXMLParseStringCtxt(doc, _("(domain_definition)"), &ctxt))) { vshError(ctl, "%s", _("Failed to get interface information")); goto cleanup; } @@ -9460,10 +9488,7 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd) goto cleanup; }
- if (vshCommandOptBool(cmd, "config")) { - flags = VIR_DOMAIN_AFFECT_CONFIG; - if (virDomainIsActive(dom) == 1) - flags |= VIR_DOMAIN_AFFECT_LIVE; + if (flags != 0) { ret = virDomainDetachDeviceFlags(dom, (char *)xmlBufferContent(xml_buf), flags);
This if seems pointless, because we usually call the *Flags() function with flags=0, but this particular function is (for example in qemu driver) calling the *Flags() function with AFFECT_LIVE. The current virsh code also adds the AFFECT_LIVE automatically, as you can see...
@@ -9479,6 +9504,7 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd) }
cleanup: + VIR_FREE(doc);
If this fixes a leak, it could be mentioned in the commit message.
virDomainFree(dom); xmlXPathFreeObject(obj); xmlXPathFreeContext(ctxt); diff --git a/tools/virsh.pod b/tools/virsh.pod index a9e8c65..ebbe201 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1876,16 +1876,23 @@ If I<--config> is specified, alter persistent configuration, effect observed on next boot, for compatibility purposes, I<--persistent> is alias of I<--config>.
-=item B<detach-interface> I<domain> I<type> [I<--mac mac>] [I<--config>] +=item B<detach-interface> I<domain> I<type> [I<--mac mac>] +[[[I<--live>] [I<--config>] | [I<--current>]] | [I<--persistent>]]
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. -If I<--config> is specified, alter persistent configuration, effect observed -on next boot, for compatibility purposes, I<--persistent> is alias of -I<--config>. + +If I<--live> is specified, affect a running domain. +If I<--config> is specified, affect the next startup of a persistent domain. +If I<--current> is specified, affect the current domain state. +Both I<--live> and I<--config> flags may be given, but I<--current> is +exclusive. Not specifying any flag is the same as specifying I<--current>.
... but it doesn't cope with this definition in the man page. I haven't investigated all the other drivers and possibilities, but this behavior shouldn't change. Martin
On 03/27/13 13:53, Martin Kletzander wrote:
On 03/21/2013 05:42 PM, Peter Krempa wrote:
Use the established approach to improve this function too. --- tools/virsh-domain.c | 52 +++++++++++++++++++++++++++++++++++++++------------- tools/virsh.pod | 15 +++++++++++---- 2 files changed, 50 insertions(+), 17 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 6741837..df72c78 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -9353,13 +9353,25 @@ static const vshCmdOptDef opts_detach_interface[] = { .help = N_("MAC address") }, {.name = "persistent", - .type = VSH_OT_ALIAS, - .help = "config" + .type = VSH_OT_BOOL, + .help = N_("make live change persistent") }, {.name = "config", .type = VSH_OT_BOOL, .help = N_("affect next boot") }, + {.name = "live", + .type = VSH_OT_BOOL, + .help = N_("affect running domain") + }, + {.name = "current", + .type = VSH_OT_BOOL, + .help = N_("affect current domain") + }, + {.name = "force", + .type = VSH_OT_BOOL, + .help = N_("force device update") + },
Adding non-used --force paramter, copy-paste error?
{.name = NULL} };
@@ -9373,12 +9385,27 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd) xmlNodePtr cur = NULL, matchNode = NULL; xmlBufferPtr xml_buf = NULL; const char *mac =NULL, *type = NULL; - char *doc; + char *doc = NULL; char buf[64]; int i = 0, diff_mac; int ret; int functionReturn = false; - unsigned int flags; + unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; + bool current = vshCommandOptBool(cmd, "current"); + bool config = vshCommandOptBool(cmd, "config"); + bool live = vshCommandOptBool(cmd, "live"); + bool persistent = vshCommandOptBool(cmd, "persistent"); + + VSH_EXCLUSIVE_OPTIONS_VAR(persistent, live);
Same as in previous patch, remove this line.
+ VSH_EXCLUSIVE_OPTIONS_VAR(persistent, current); + + VSH_EXCLUSIVE_OPTIONS_VAR(current, live); + VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + + if (config || persistent) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE;
if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false; @@ -9389,13 +9416,14 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptStringReq(ctl, cmd, "mac", &mac) < 0) goto cleanup;
- doc = virDomainGetXMLDesc(dom, 0); - if (!doc) + if (persistent && + virDomainIsActive(dom) == 1) + flags |= VIR_DOMAIN_AFFECT_LIVE; + + if (!(doc = virDomainGetXMLDesc(dom, 0))) goto cleanup;
- xml = virXMLParseStringCtxt(doc, _("(domain_definition)"), &ctxt); - VIR_FREE(doc); - if (!xml) { + if (!(xml = virXMLParseStringCtxt(doc, _("(domain_definition)"), &ctxt))) { vshError(ctl, "%s", _("Failed to get interface information")); goto cleanup; } @@ -9460,10 +9488,7 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd) goto cleanup; }
- if (vshCommandOptBool(cmd, "config")) { - flags = VIR_DOMAIN_AFFECT_CONFIG; - if (virDomainIsActive(dom) == 1) - flags |= VIR_DOMAIN_AFFECT_LIVE; + if (flags != 0) { ret = virDomainDetachDeviceFlags(dom, (char *)xmlBufferContent(xml_buf), flags);
This if seems pointless, because we usually call the *Flags() function with flags=0, but this particular function is (for example in qemu driver) calling the *Flags() function with AFFECT_LIVE. The current virsh code also adds the AFFECT_LIVE automatically, as you can see...
@@ -9479,6 +9504,7 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd) }
cleanup: + VIR_FREE(doc);
If this fixes a leak, it could be mentioned in the commit message.
virDomainFree(dom); xmlXPathFreeObject(obj); xmlXPathFreeContext(ctxt); diff --git a/tools/virsh.pod b/tools/virsh.pod index a9e8c65..ebbe201 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1876,16 +1876,23 @@ If I<--config> is specified, alter persistent configuration, effect observed on next boot, for compatibility purposes, I<--persistent> is alias of I<--config>.
-=item B<detach-interface> I<domain> I<type> [I<--mac mac>] [I<--config>] +=item B<detach-interface> I<domain> I<type> [I<--mac mac>] +[[[I<--live>] [I<--config>] | [I<--current>]] | [I<--persistent>]]
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. -If I<--config> is specified, alter persistent configuration, effect observed -on next boot, for compatibility purposes, I<--persistent> is alias of -I<--config>. + +If I<--live> is specified, affect a running domain. +If I<--config> is specified, affect the next startup of a persistent domain. +If I<--current> is specified, affect the current domain state. +Both I<--live> and I<--config> flags may be given, but I<--current> is +exclusive. Not specifying any flag is the same as specifying I<--current>.
I changed the last sentence to: When no flag is specified legacy API is used whose behavior depends on the hypervisor driver. Peter
Use the established approach to improve this function too. --- tools/virsh-domain.c | 46 +++++++++++++++++++++++++++++++++++++--------- tools/virsh.pod | 15 +++++++++++---- 2 files changed, 48 insertions(+), 13 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index df72c78..7079066 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -9167,14 +9167,27 @@ static const vshCmdOptDef opts_detach_device[] = { .help = N_("XML file") }, {.name = "persistent", - .type = VSH_OT_ALIAS, - .help = "config" + .type = VSH_OT_BOOL, + .help = N_("make live change persistent") }, {.name = "config", .type = VSH_OT_BOOL, .help = N_("affect next boot") }, + {.name = "live", + .type = VSH_OT_BOOL, + .help = N_("affect running domain") + }, + {.name = "current", + .type = VSH_OT_BOOL, + .help = N_("affect current domain") + }, + {.name = "force", + .type = VSH_OT_BOOL, + .help = N_("force device update") + }, {.name = NULL} + }; static bool @@ -9185,11 +9198,30 @@ cmdDetachDevice(vshControl *ctl, const vshCmd *cmd) char *buffer = NULL; int ret; bool funcRet = false; - unsigned int flags; + bool current = vshCommandOptBool(cmd, "current"); + bool config = vshCommandOptBool(cmd, "config"); + bool live = vshCommandOptBool(cmd, "live"); + bool persistent = vshCommandOptBool(cmd, "persistent"); + unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; + + VSH_EXCLUSIVE_OPTIONS_VAR(persistent, live); + VSH_EXCLUSIVE_OPTIONS_VAR(persistent, current); + + VSH_EXCLUSIVE_OPTIONS_VAR(current, live); + VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + + if (config || persistent) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false; + if (persistent && + virDomainIsActive(dom) == 1) + flags |= VIR_DOMAIN_AFFECT_LIVE; + if (vshCommandOptStringReq(ctl, cmd, "file", &from) < 0) goto cleanup; @@ -9198,14 +9230,10 @@ cmdDetachDevice(vshControl *ctl, const vshCmd *cmd) goto cleanup; } - if (vshCommandOptBool(cmd, "config")) { - flags = VIR_DOMAIN_AFFECT_CONFIG; - if (virDomainIsActive(dom) == 1) - flags |= VIR_DOMAIN_AFFECT_LIVE; + if (flags != 0) ret = virDomainDetachDeviceFlags(dom, buffer, flags); - } else { + else ret = virDomainDetachDevice(dom, buffer); - } if (ret < 0) { vshError(ctl, _("Failed to detach device from %s"), from); diff --git a/tools/virsh.pod b/tools/virsh.pod index ebbe201..760b164 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1858,16 +1858,23 @@ B<Note>: the optional target value is the name of a device to be created as the back-end on the node. If not provided a device named "vnetN" or "vifN" will be created automatically. -=item B<detach-device> I<domain> I<FILE> [I<--config>] +=item B<detach-device> I<domain> I<FILE> +[[[I<--live>] [I<--config>] | [I<--current>]] | [I<--persistent>]] Detach a device from the domain, takes the same kind of XML descriptions as command B<attach-device>. -If I<--config> is specified, alter persistent configuration, effect observed -on next boot, for compatibility purposes, I<--persistent> is alias of -I<--config>. For passthrough host devices, see also B<nodedev-reattach>, needed if the device does not use managed mode. +If I<--live> is specified, affect a running domain. +If I<--config> is specified, affect the next startup of a persistent domain. +If I<--current> is specified, affect the current domain state. +Both I<--live> and I<--config> flags may be given, but I<--current> is +exclusive. Not specifying any flag is the same as specifying I<--current>. + +For compatibility purposes, I<--persistent> behaves like I<--config> for +an offline domain, and like I<--live> I<--config> for a running domain. + =item B<detach-disk> I<domain> I<target> [I<--config>] Detach a disk device from a domain. The I<target> is the device as seen -- 1.8.1.5
Use the established approach to improve this function too. --- tools/virsh-domain.c | 59 +++++++++++++++++++++++++++++++++++++--------------- tools/virsh.pod | 15 +++++++++---- 2 files changed, 53 insertions(+), 21 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 7079066..35c3961 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -9782,13 +9782,25 @@ static const vshCmdOptDef opts_detach_disk[] = { .help = N_("target of disk device") }, {.name = "persistent", - .type = VSH_OT_ALIAS, - .help = "config" + .type = VSH_OT_BOOL, + .help = N_("make live change persistent") }, {.name = "config", .type = VSH_OT_BOOL, .help = N_("affect next boot") }, + {.name = "live", + .type = VSH_OT_BOOL, + .help = N_("affect running domain") + }, + {.name = "current", + .type = VSH_OT_BOOL, + .help = N_("affect current domain") + }, + {.name = "force", + .type = VSH_OT_BOOL, + .help = N_("force device update") + }, {.name = NULL} }; @@ -9801,8 +9813,23 @@ cmdDetachDisk(vshControl *ctl, const vshCmd *cmd) char *doc = NULL; int ret; bool functionReturn = false; - unsigned int flags; xmlNodePtr disk_node = NULL; + bool current = vshCommandOptBool(cmd, "current"); + bool config = vshCommandOptBool(cmd, "config"); + bool live = vshCommandOptBool(cmd, "live"); + bool persistent = vshCommandOptBool(cmd, "persistent"); + unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; + + VSH_EXCLUSIVE_OPTIONS_VAR(persistent, live); + VSH_EXCLUSIVE_OPTIONS_VAR(persistent, current); + + VSH_EXCLUSIVE_OPTIONS_VAR(current, live); + VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + + if (config || persistent) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false; @@ -9810,10 +9837,13 @@ cmdDetachDisk(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptStringReq(ctl, cmd, "target", &target) < 0) goto cleanup; - doc = virDomainGetXMLDesc(dom, 0); - if (!doc) + if (!(doc = virDomainGetXMLDesc(dom, 0))) goto cleanup; + if (persistent && + virDomainIsActive(dom) == 1) + flags |= VIR_DOMAIN_AFFECT_LIVE; + if (!(disk_node = vshFindDisk(doc, target, VSH_FIND_DISK_NORMAL))) goto cleanup; @@ -9821,24 +9851,19 @@ cmdDetachDisk(vshControl *ctl, const vshCmd *cmd) VSH_PREPARE_DISK_XML_NONE))) goto cleanup; - if (vshCommandOptBool(cmd, "config")) { - flags = VIR_DOMAIN_AFFECT_CONFIG; - if (virDomainIsActive(dom) == 1) - flags |= VIR_DOMAIN_AFFECT_LIVE; - ret = virDomainDetachDeviceFlags(dom, - disk_xml, - flags); - } else { + if (flags != 0) + ret = virDomainDetachDeviceFlags(dom, disk_xml, flags); + else ret = virDomainDetachDevice(dom, disk_xml); - } if (ret != 0) { vshError(ctl, "%s", _("Failed to detach disk")); - } else { - vshPrint(ctl, "%s", _("Disk detached successfully\n")); - functionReturn = true; + goto cleanup; } + vshPrint(ctl, "%s", _("Disk detached successfully\n")); + functionReturn = true; + cleanup: xmlFreeNode(disk_node); VIR_FREE(disk_xml); diff --git a/tools/virsh.pod b/tools/virsh.pod index 760b164..04df31b 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1875,13 +1875,20 @@ exclusive. Not specifying any flag is the same as specifying I<--current>. For compatibility purposes, I<--persistent> behaves like I<--config> for an offline domain, and like I<--live> I<--config> for a running domain. -=item B<detach-disk> I<domain> I<target> [I<--config>] +=item B<detach-disk> I<domain> I<target> +[[[I<--live>] [I<--config>] | [I<--current>]] | [I<--persistent>]] Detach a disk device from a domain. The I<target> is the device as seen from the domain. -If I<--config> is specified, alter persistent configuration, effect observed -on next boot, for compatibility purposes, I<--persistent> is alias of -I<--config>. + +If I<--live> is specified, affect a running domain. +If I<--config> is specified, affect the next startup of a persistent domain. +If I<--current> is specified, affect the current domain state. +Both I<--live> and I<--config> flags may be given, but I<--current> is +exclusive. Not specifying any flag is the same as specifying I<--current>. + +For compatibility purposes, I<--persistent> behaves like I<--config> for +an offline domain, and like I<--live> I<--config> for a running domain. =item B<detach-interface> I<domain> I<type> [I<--mac mac>] [[[I<--live>] [I<--config>] | [I<--current>]] | [I<--persistent>]] -- 1.8.1.5
participants (4)
-
Eric Blake -
Laine Stump -
Martin Kletzander -
Peter Krempa