[libvirt] [PATCHv2 1/4] virsh: Fix semantics of --config for "update-device" command

[Laine Stump]

On 03/31/2013 05:22 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 boot was affected. This isn't
> true if the machine is running.

You should probably change this comment to make it clear that you're
changing the behavior of the option, rather than the documentation of
what it does.

After our discussion last week, I do agree that, although this is a
change in behavior of an already-released command, it's acceptable
because 1) it was different behavior from all other commands using
--config, and 2) it was documented as behaving as all other commands.


>
> This patch adds the full --live, --config, --current infrastructure and
> tweaks stuff to correctly support the obsolete --persistent flag.
> ---
>
> Notes:
>     Version 2:
>     - note in the docs that semantics of the flags were fixed

ACK once you note in the commit log that the code was fixed, not the
documentation.

>
>  tools/virsh-domain.c | 40 +++++++++++++++++++++++++++++-----------
>  tools/virsh.pod      | 24 +++++++++++++++++-------
>  2 files changed, 46 insertions(+), 18 deletions(-)
>
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index fa5ab3d..2a228f5 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,21 @@ 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, 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 +9297,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 e7e82e3..04ea542 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -1888,18 +1888,28 @@ 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.
> +
> +Note that older versions of virsh used I<--config> as an alias for
> +I<--persistent>.
> +
>  =item B<change-media> I<domain> I<path> [I<--eject>] [I<--insert>]
>  [I<--update>] [I<source>] [I<--force>] [[I<--live>] [I<--config>] | [I<--current>]]
>