Re: [PATCH v8 resend 4/4] allow "virsh dump --memory-only" specify dump format

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 03/22/2014 09:51 PM, Wen Congyang wrote:
> From: Qiao Nuohan <qiaonuohan@xxxxxxxxxxxxxx>
> 
> This patch adds "[--format] <string>" to "virsh dump --memory-only", which is
> changed to use the new virDomainCoreDumpWithFormat API. And "--compress" is
> added as an alias for "--format kdump-zlib".
> 
> Signed-off-by: Qiao Nuohan <qiaonuohan@xxxxxxxxxxxxxx>
> ---
>  tools/virsh-domain.c | 47 ++++++++++++++++++++++++++++++++++++++++++++---
>  tools/virsh.pod      |  6 ++++++
>  2 files changed, 50 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 0664774..d897c72 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -4490,6 +4490,14 @@ static const vshCmdOptDef opts_dump[] = {
>       .type = VSH_OT_BOOL,
>       .help = N_("dump domain's memory only")
>      },
> +    {.name = "compress",
> +     .type = VSH_OT_ALIAS,
> +     .help = "format=kdump-zlib",
> +    },

Remember that the alias mechanism intentionally results in undocumented
support.  Observe:

$ tools/virsh dump --help | grep compress
$ tools/virsh dump --help | grep format
    dump [--live] [--crash] [--bypass-cache] [--reset] <domain> <file>
[--verbose] [--memory-only] [<format>]
    [--format] <string>  specify the format of memory-only dump
$

To date, we have only used aliases to fix warts in old interfaces while
avoiding back-compat breaks.  But you are attempting to use the alias as
a brand new interface.  I'd much rather NOT push the alias.
Undocumented interfaces are evil, and should only exist when back-compat
is at stake.  But here, we've never had --compress in the past, so
back-compat is not an issue.

If you absolutely must have --compress (and can't stand to type
--format=kdump-zlib in your scripts), it would be better to do that as a
followup patch with a lot more justification and while ensuring it is a
documented interface.  Or even better would be adding support for
generic user aliases (where I could create an alias 'd' for 'dump
--format=kdump-zlib', then use 'virsh d $dom').


>  
> -    if (virDomainCoreDump(dom, to, flags) < 0) {
> -        vshError(ctl, _("Failed to core dump domain %s to %s"), name, to);
> -        goto out;
> +    if (vshCommandOptBool(cmd, "format")) {
> +        if (!(flags & VIR_DUMP_MEMORY_ONLY)) {
> +            vshError(ctl, "%s", _("--format only work with --memory-only"));
> +            goto out;

Grammar is off.


> +
> +        if (vshCommandOptString(cmd, "format", &format)) {
> +            if (STREQ(format, "kdump-zlib"))
> +                dumpformat = VIR_DOMAIN_CORE_DUMP_FORMAT_KDUMP_ZLIB;
> +            else if (STREQ(format, "kdump-lzo"))
> +                dumpformat = VIR_DOMAIN_CORE_DUMP_FORMAT_KDUMP_LZO;
> +            else if (STREQ(format, "kdump-snappy"))
> +                dumpformat = VIR_DOMAIN_CORE_DUMP_FORMAT_KDUMP_SNAPPY;
> +            else if (STREQ(format, "elf"))
> +                dumpformat = VIR_DOMAIN_CORE_DUMP_FORMAT_RAW;
> +            else {

HACKING says that if the 'else' side has {}, the 'if' side must as well.

> +                vshError(ctl, _("format '%s' is not supported, expecting "
> +                                "'kdump-zlib', 'kdump-lzo', 'kdump-snappy' "
> +                                "or 'elf'."), format);

We tend to avoid trailing . in messages

I've pushed your series, with this squashed in:


diff --git i/tools/virsh-domain.c w/tools/virsh-domain.c
index d897c72..f2856a5 100644
--- i/tools/virsh-domain.c
+++ w/tools/virsh-domain.c
@@ -4490,10 +4490,6 @@ static const vshCmdOptDef opts_dump[] = {
      .type = VSH_OT_BOOL,
      .help = N_("dump domain's memory only")
     },
-    {.name = "compress",
-     .type = VSH_OT_ALIAS,
-     .help = "format=kdump-zlib",
-    },
     {.name = "format",
      .type = VSH_OT_DATA,
      .help = N_("specify the format of memory-only dump")
@@ -4540,23 +4536,23 @@ doDump(void *opaque)

     if (vshCommandOptBool(cmd, "format")) {
         if (!(flags & VIR_DUMP_MEMORY_ONLY)) {
-            vshError(ctl, "%s", _("--format only work with
--memory-only"));
+            vshError(ctl, "%s", _("--format only works with
--memory-only"));
             goto out;
         }

         if (vshCommandOptString(cmd, "format", &format)) {
-            if (STREQ(format, "kdump-zlib"))
+            if (STREQ(format, "kdump-zlib")) {
                 dumpformat = VIR_DOMAIN_CORE_DUMP_FORMAT_KDUMP_ZLIB;
-            else if (STREQ(format, "kdump-lzo"))
+            } else if (STREQ(format, "kdump-lzo")) {
                 dumpformat = VIR_DOMAIN_CORE_DUMP_FORMAT_KDUMP_LZO;
-            else if (STREQ(format, "kdump-snappy"))
+            } else if (STREQ(format, "kdump-snappy")) {
                 dumpformat = VIR_DOMAIN_CORE_DUMP_FORMAT_KDUMP_SNAPPY;
-            else if (STREQ(format, "elf"))
+            } else if (STREQ(format, "elf")) {
                 dumpformat = VIR_DOMAIN_CORE_DUMP_FORMAT_RAW;
-            else {
+            } else {
                 vshError(ctl, _("format '%s' is not supported, expecting "
                                 "'kdump-zlib', 'kdump-lzo',
'kdump-snappy' "
-                                "or 'elf'."), format);
+                                "or 'elf'"), format);
                 goto out;
             }
         }
diff --git i/tools/virsh.pod w/tools/virsh.pod
index 7e3ae5a..20352cb 100644
--- i/tools/virsh.pod
+++ w/tools/virsh.pod
@@ -1027,8 +1027,7 @@ useful if the domain uses host devices directly.
 I<--format> I<string> is used to specify the format of 'memory-only'
 dump, and I<string> can be one of them: elf, kdump-zlib(kdump-compressed
 format with zlib-compressed), kdump-lzo(kdump-compressed format with
-lzo-compressed), kdump-snappy(kdump-compressed format with
snappy-compressed)
-I<--compress> is an alias for I<--format> I<kdump-zlib>.
+lzo-compressed), kdump-snappy(kdump-compressed format with
snappy-compressed).

 The progress may be monitored using B<domjobinfo> virsh command and
canceled
 with B<domjobabort> command (sent by another virsh instance). Another
option


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]