[libvirt] [PATCH 1/2] maint: avoid unwanted newline at end of diagnostic

While I was in the virtual vicinity, Jiri mentioned the possibility of detecting one more minor formatting problem: the stray newline at the end of a diagnostic. Here are two corrections and the heuristic that caught them (and that will prevent introduction of some new ones -- but not if the end of the message is more than 2 lines after the function name). Currently there are no false positives.
From 122b1e31fb33c092a53802b56a0f2f5586c95bd5 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 20 May 2010 11:12:17 +0200 Subject: [PATCH 1/2] maint: remove unwanted newline at end of diagnostic
* src/xen/xend_internal.c (xenDaemonDomainDefineXML): Remove \n. * src/network/bridge_driver.c (networkAddMasqueradingIptablesRules): Likewise. --- src/network/bridge_driver.c | 2 +- src/xen/xend_internal.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 3b9b4f4..5d7ef19 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -642,7 +642,7 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver, network->def->network, network->def->forwardDev))) { virReportSystemError(err, - _("failed to add iptables rule to enable masquerading to '%s'\n"), + _("failed to add iptables rule to enable masquerading to '%s'"), network->def->forwardDev ? network->def->forwardDev : NULL); goto masqerr3; } diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index ea5addd..a203a8d 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -4731,7 +4731,7 @@ virDomainPtr xenDaemonDomainDefineXML(virConnectPtr conn, const char *xmlDesc) { VIR_FREE(sexpr); if (ret != 0) { virXendError(VIR_ERR_XEN_CALL, - _("Failed to create inactive domain %s\n"), def->name); + _("Failed to create inactive domain %s"), def->name); goto error; } -- 1.7.1.259.g3aef8
From 532672f3a0c53213609dab49df2e7efcaf0eb594 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 20 May 2010 11:13:41 +0200 Subject: [PATCH 2/2] maint: prohibit newline at end of diagnostic
* cfg.mk (sc_prohibit_newline_at_end_of_diagnostic): New rule. Idea proposed by Jiri Denemark. --- cfg.mk | 14 ++++++++++++++ 1 files changed, 14 insertions(+), 0 deletions(-) diff --git a/cfg.mk b/cfg.mk index 2909e3e..b024c75 100644 --- a/cfg.mk +++ b/cfg.mk @@ -391,6 +391,20 @@ sc_libvirt_unmarked_diagnostics: { echo '$(ME): found unmarked diagnostic(s)' 1>&2; \ exit 1; } || : +# Like the above, but prohibit a newline at the end of a diagnostic. +# This is subject to false positives partly because it naively looks for +# `\n"', which may not be the end of the string, and also because it takes +# two lines of context (the -A2) after the line with the function name. +# FIXME: this rule might benefit from a separate function list, in case +# there are functions to which this one applies but that do not get marked +# diagnostics. +sc_prohibit_newline_at_end_of_diagnostic: + @grep -A2 -nE \ + '\<$(func_re) *\(' $$($(VC_LIST_EXCEPT)) \ + | grep '\\n"' \ + && { echo '$(ME): newline at end of message(s)' 1>&2; \ + exit 1; } || : + # Disallow trailing blank lines. sc_prohibit_trailing_blank_lines: @$(VC_LIST_EXCEPT) | xargs perl -ln -0777 -e \ -- 1.7.1.259.g3aef8

While I was in the virtual vicinity, Jiri mentioned the possibility of detecting one more minor formatting problem: the stray newline at the end of a diagnostic. Here are two corrections and the heuristic that caught them (and that will prevent introduction of some new ones -- but not if the end of the message is more than 2 lines after the function name). Currently there are no false positives.
From 122b1e31fb33c092a53802b56a0f2f5586c95bd5 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 20 May 2010 11:12:17 +0200 Subject: [PATCH 1/2] maint: remove unwanted newline at end of diagnostic
* src/xen/xend_internal.c (xenDaemonDomainDefineXML): Remove \n. * src/network/bridge_driver.c (networkAddMasqueradingIptablesRules): Likewise.
From 532672f3a0c53213609dab49df2e7efcaf0eb594 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 20 May 2010 11:13:41 +0200 Subject: [PATCH 2/2] maint: prohibit newline at end of diagnostic
* cfg.mk (sc_prohibit_newline_at_end_of_diagnostic): New rule. Idea proposed by Jiri Denemark.
ACK to both patches. While the syntax check implementation is not perfect, it's good enough for now and can be enhanced if there is a desperate need for that. Jirka

Jiri Denemark wrote: ...
Subject: [PATCH 1/2] maint: remove unwanted newline at end of diagnostic
* src/xen/xend_internal.c (xenDaemonDomainDefineXML): Remove \n. * src/network/bridge_driver.c (networkAddMasqueradingIptablesRules): Likewise.
From 532672f3a0c53213609dab49df2e7efcaf0eb594 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 20 May 2010 11:13:41 +0200 Subject: [PATCH 2/2] maint: prohibit newline at end of diagnostic
* cfg.mk (sc_prohibit_newline_at_end_of_diagnostic): New rule. Idea proposed by Jiri Denemark.
ACK to both patches.
While the syntax check implementation is not perfect, it's good enough for now and can be enhanced if there is a desperate need for that.
Thanks. Pushed.

On 05/20/2010 05:18 AM, Jim Meyering wrote:
From 122b1e31fb33c092a53802b56a0f2f5586c95bd5 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 20 May 2010 11:12:17 +0200 Subject: [PATCH 1/2] maint: remove unwanted newline at end of diagnostic
* src/xen/xend_internal.c (xenDaemonDomainDefineXML): Remove \n. * src/network/bridge_driver.c (networkAddMasqueradingIptablesRules): Likewise. --- src/network/bridge_driver.c | 2 +- src/xen/xend_internal.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 3b9b4f4..5d7ef19 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -642,7 +642,7 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver, network->def->network, network->def->forwardDev))) { virReportSystemError(err, - _("failed to add iptables rule to enable masquerading to '%s'\n"), + _("failed to add iptables rule to enable masquerading to '%s'"), network->def->forwardDev ? network->def->forwardDev : NULL); goto masqerr3; } diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index ea5addd..a203a8d 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -4731,7 +4731,7 @@ virDomainPtr xenDaemonDomainDefineXML(virConnectPtr conn, const char *xmlDesc) { VIR_FREE(sexpr); if (ret != 0) { virXendError(VIR_ERR_XEN_CALL, - _("Failed to create inactive domain %s\n"), def->name); + _("Failed to create inactive domain %s"), def->name); goto error; }
ACK to this part, certainly. I'm not sure a new syntax-check rule (which may have false positives) is worth it; the fact that there are so few occurrences of the problem in the codebase seems to say that it's not a huge problem, and I don't want to make sytnax-check fail for people for bogus reasons. -- Chris Lalancette

Chris Lalancette wrote: ...
- _("Failed to create inactive domain %s\n"), def->name); + _("Failed to create inactive domain %s"), def->name); goto error; }
ACK to this part, certainly.
I'm not sure a new syntax-check rule (which may have false positives) is worth it; the fact that there are so few occurrences of the problem in the codebase seems to say that it's not a huge problem, and I don't want to make sytnax-check fail for people for bogus reasons.
Hi Chris, Thanks for the review. I already pushed it, based on a prior ack. If/when problems arise, we'll deal with it by improving the check, allowing exemption(s) via its .x-sc... file, or simply by removing the test altogether.
participants (3)
-
Chris Lalancette
-
Jim Meyering
-
Jiri Denemark