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