[libvirt] [PATCH] virsh: Print error message if argument parsing fails for cmdNodesuspend
If parsing of arguments failed, virsh did silently exit returning and error state, but not specifying the possible problem. * tools/virsh: cmdNodesuspend: - error handling added --- tools/virsh.c | 12 +++++++++--- 1 files changed, 9 insertions(+), 3 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index d58b827..ebda248 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -5270,14 +5270,20 @@ cmdNodeSuspend(vshControl *ctl, const vshCmd *cmd) if (!vshConnectionUsability(ctl, ctl->conn)) return false; - if (vshCommandOptString(cmd, "target", &target) < 0) + if (vshCommandOptString(cmd, "target", &target) < 0) { + vshError(ctl, _("Invalid suspend target argument")); return false; + } - if (vshCommandOptLongLong(cmd, "duration", &duration) < 0) + if (vshCommandOptLongLong(cmd, "duration", &duration) < 0) { + vshError(ctl, _("Invalid suspend duration argument")); return false; + } - if (vshCommandOptUInt(cmd, "flags", &flags) < 0) + if (vshCommandOptUInt(cmd, "flags", &flags) < 0) { + vshError(ctl, _("Invalid suspend flags argument")); return false; + } if (STREQ(target, "mem")) suspendTarget = VIR_NODE_SUSPEND_TARGET_MEM; -- 1.7.3.4
On 13.12.2011 14:53, Peter Krempa wrote:
If parsing of arguments failed, virsh did silently exit returning and error state, but not specifying the possible problem.
* tools/virsh: cmdNodesuspend: - error handling added --- tools/virsh.c | 12 +++++++++--- 1 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index d58b827..ebda248 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -5270,14 +5270,20 @@ cmdNodeSuspend(vshControl *ctl, const vshCmd *cmd) if (!vshConnectionUsability(ctl, ctl->conn)) return false;
- if (vshCommandOptString(cmd, "target", &target) < 0) + if (vshCommandOptString(cmd, "target", &target) < 0) { + vshError(ctl, _("Invalid suspend target argument")); I'd do s/suspend // in all strings you're adding.
ACK with that nit fixed. Michal
On 12/13/2011 03:31 PM, Michal Privoznik wrote:
On 13.12.2011 14:53, Peter Krempa wrote:
If parsing of arguments failed, virsh did silently exit returning and error state, but not specifying the possible problem.
* tools/virsh: cmdNodesuspend: - error handling added --- tools/virsh.c | 12 +++++++++--- 1 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index d58b827..ebda248 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -5270,14 +5270,20 @@ cmdNodeSuspend(vshControl *ctl, const vshCmd *cmd) if (!vshConnectionUsability(ctl, ctl->conn)) return false;
- if (vshCommandOptString(cmd, "target",&target)< 0) + if (vshCommandOptString(cmd, "target",&target)< 0) { + vshError(ctl, _("Invalid suspend target argument")); I'd do s/suspend // in all strings you're adding.
ACK with that nit fixed.
Yeah, that actually looks better. Thanks. Fixed && pushed Peter
Michal
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 12/13/2011 07:31 AM, Michal Privoznik wrote:
- if (vshCommandOptString(cmd, "target", &target) < 0) + if (vshCommandOptString(cmd, "target", &target) < 0) { + vshError(ctl, _("Invalid suspend target argument"));
I'd do s/suspend // in all strings you're adding.
Compilation without NLS will trigger gcc warnings that you are using a printf-style interface without any %. Write this as vshError(ctl, "%s", _("Invalid target argument")). -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Dňa 13.12.2011 16:08, Eric Blake wrote / napísal(a):
On 12/13/2011 07:31 AM, Michal Privoznik wrote:
- if (vshCommandOptString(cmd, "target",&target)< 0) + if (vshCommandOptString(cmd, "target",&target)< 0) { + vshError(ctl, _("Invalid suspend target argument"));
I'd do s/suspend // in all strings you're adding.
Compilation without NLS will trigger gcc warnings that you are using a printf-style interface without any %. Write this as vshError(ctl, "%s", _("Invalid target argument")).
Oh! Now I understand why there's used notation "%s", "some const string" throughout virsh. A quick grep on virsh's source revealed more places where this happens and even some strings printed without the translation macro. (Well, I was using that format in my previous patches ...). I'll look for them and try to fix all the locations in a separate patch. Peter
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 12/13/2011 03:24 PM, Peter Krempa wrote:
Compilation without NLS will trigger gcc warnings that you are using a printf-style interface without any %. Write this as vshError(ctl, "%s", _("Invalid target argument")).
Oh! Now I understand why there's used notation "%s", "some const string" throughout virsh.
I wish we could get gcc to warn even when compiling with NLS. clang also issues the warnings about a constant format string, regardless of whether NLS is enabled, but we don't compile under clang as often. (The 'documented' reason that gcc warns only when you disable NLS is that with NLS, the string is hidden inside a function call of gettext(), but without NLS, the _() macro is a no-op. But the fact that gcc can see through the gettext() call to warn about %d vs. long mismatch makes me say that this is a weak cop-out argument from the gcc camp, and that they should fix their consistency bug).
A quick grep on virsh's source revealed more places where this happens and even some strings printed without the translation macro. (Well, I was using that format in my previous patches ...). I'll look for them and try to fix all the locations in a separate patch.
Sure, bundle them all in as a single, separate cleanup patch; it won't be the first time we've done that (see, for example, commit 991be604). Alas, I don't know of any way to teach 'make syntax-check' to be smarter about it, so it probably also won't be the last. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Dec 13, 2011 at 03:32:12PM -0700, Eric Blake wrote:
On 12/13/2011 03:24 PM, Peter Krempa wrote:
Compilation without NLS will trigger gcc warnings that you are using a printf-style interface without any %. Write this as vshError(ctl, "%s", _("Invalid target argument")).
Oh! Now I understand why there's used notation "%s", "some const string" throughout virsh.
I wish we could get gcc to warn even when compiling with NLS. clang also issues the warnings about a constant format string, regardless of whether NLS is enabled, but we don't compile under clang as often.
Perhaps the best thing todo is to simply change autobuild.sh to run with --disable-nls. Autobuild already results in 2 complete builds, one direct and one via rpmbuild, so adding --disable-nls to the first direct build will improve our coverage here Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Michal Privoznik -
Peter Krempa