* [RFC/PATCH] imap-send: Code correctness flagged by clang
@ 2010-08-07 12:12 Ævar Arnfjörð Bjarmason
2010-08-07 21:04 ` Jonathan Nieder
0 siblings, 1 reply; 5+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2010-08-07 12:12 UTC (permalink / raw)
To: git
Cc: Mike McCormack, Benjamin Kramer, Ævar Arnfjörð Bjarmason
Clang 1.1 flagged the following issues in imap-send.c, this change
fixes the warnings by moving some code around:
imap-send.c:548:27: warning: data argument not used by format string [-Wformat-extra-args]
cmd->tag, cmd->cmd, cmd->cb.dlen);
^
Here the sprintf format didn't use the cmd->cb.dlen argument if
cmd->cb.data was false. Change the code to use a if/else instead of a
two-level ternary to work it. This code was introduced with imap-send
itself in f2561fda.
imap-send.c:1089:41: warning: conversion specifies type 'unsigned short' but the argument has type 'int' [-Wformat]
snprintf(portstr, sizeof(portstr), "%hu", srvc->port);
~~^ ~~~~~~~~~~
Here sprintf is being given an int with a %hu format. Cast the
srvc->port to unsigned short to work it. This code was introduced in
94ad2437 to add IPv6 support.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
imap-send.c | 13 +++++++++----
1 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/imap-send.c b/imap-send.c
index 1a577a0..4b25375 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -543,9 +543,14 @@ static struct imap_cmd *v_issue_imap_cmd(struct imap_store *ctx,
while (imap->literal_pending)
get_cmd_result(ctx, NULL);
- bufl = nfsnprintf(buf, sizeof(buf), cmd->cb.data ? CAP(LITERALPLUS) ?
- "%d %s{%d+}\r\n" : "%d %s{%d}\r\n" : "%d %s\r\n",
- cmd->tag, cmd->cmd, cmd->cb.dlen);
+ if (cmd->cb.data) {
+ bufl = nfsnprintf(buf, sizeof(buf),
+ CAP(LITERALPLUS) ? "%d %s{%d+}\r\n" : "%d %s{%d}\r\n",
+ cmd->tag, cmd->cmd, cmd->cb.dlen);
+ } else {
+ bufl = nfsnprintf(buf, sizeof(buf), "%d %s\r\n", cmd->tag, cmd->cmd);
+ }
+
if (Verbose) {
if (imap->num_in_progress)
printf("(%d in progress) ", imap->num_in_progress);
@@ -1086,7 +1091,7 @@ static struct store *imap_open_store(struct imap_server_conf *srvc)
int gai;
char portstr[6];
- snprintf(portstr, sizeof(portstr), "%hu", srvc->port);
+ snprintf(portstr, sizeof(portstr), "%hu", (unsigned short)srvc->port);
memset(&hints, 0, sizeof(hints));
hints.ai_socktype = SOCK_STREAM;
--
1.7.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [RFC/PATCH] imap-send: Code correctness flagged by clang
2010-08-07 12:12 [RFC/PATCH] imap-send: Code correctness flagged by clang Ævar Arnfjörð Bjarmason
@ 2010-08-07 21:04 ` Jonathan Nieder
2010-08-07 22:53 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 5+ messages in thread
From: Jonathan Nieder @ 2010-08-07 21:04 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Mike McCormack, Benjamin Kramer
Ævar Arnfjörð Bjarmason wrote:
> [Subject: imap-send: Code correctness flagged by clang]
>
> Clang 1.1 flagged the following issues in imap-send.c, this change
> fixes the warnings by moving some code around:
>
> imap-send.c:548:27: warning: data argument not used by format string [-Wformat-extra-args]
> cmd->tag, cmd->cmd, cmd->cb.dlen);
> ^
>
> Here the sprintf format didn't use the cmd->cb.dlen argument if
> cmd->cb.data was false. Change the code to use a if/else instead of a
> two-level ternary to work it. This code was introduced with imap-send
> itself in f2561fda.
>
> imap-send.c:1089:41: warning: conversion specifies type 'unsigned short' but the argument has type 'int' [-Wformat]
> snprintf(portstr, sizeof(portstr), "%hu", srvc->port);
> ~~^ ~~~~~~~~~~
>
> Here sprintf is being given an int with a %hu format. Cast the
> srvc->port to unsigned short to work it. This code was introduced in
> 94ad2437 to add IPv6 support.
Nitpick: that this was found by clang is probably not the first thing
a person trying to figure out what the patch does needs to know.
Maybe:
Subject: imap-send: Fix sprintf usage
When composing a command for the imap server, imap-send
uses a single nfsnprintf() invocation for brevity
instead of dealing separately with the case when there
is a message to be sent and the case when there isn’t.
The unused argument in the second case, while valid,
is confusing for static analyzers and human readers.
v1.6.4-rc0~117 (imap-send: add support for IPv6, 2009-05-25)
mistakenly used %hu as the format for an int “port”, by
analogy with existing usage for the unsigned short
“addr.sin_port”. Use %d instead.
Noticed with clang.
> +++ b/imap-send.c
> @@ -543,9 +543,14 @@ static struct imap_cmd *v_issue_imap_cmd(struct imap_store *ctx,
> while (imap->literal_pending)
> get_cmd_result(ctx, NULL);
>
> - bufl = nfsnprintf(buf, sizeof(buf), cmd->cb.data ? CAP(LITERALPLUS) ?
> - "%d %s{%d+}\r\n" : "%d %s{%d}\r\n" : "%d %s\r\n",
> - cmd->tag, cmd->cmd, cmd->cb.dlen);
> + if (cmd->cb.data) {
> + bufl = nfsnprintf(buf, sizeof(buf),
> + CAP(LITERALPLUS) ? "%d %s{%d+}\r\n" : "%d %s{%d}\r\n",
> + cmd->tag, cmd->cmd, cmd->cb.dlen);
> + } else {
> + bufl = nfsnprintf(buf, sizeof(buf), "%d %s\r\n", cmd->tag, cmd->cmd);
> + }
> +
Hmm, maybe this would be easier to read:
if (!cmd->cb.data)
bufl = nfsnprintf(buf, sizeof(buf), "%d %s\r\n", cmd->tag, cmd->cmd);
else
bufl = nfsnprintf(buf, sizeof(buf), "%d %s{%d%s}\r\n",
cmd->tag, cmd->cmd, cmd->cb.dlen,
CAP(LITERALPLUS) ? "+" : "");
i.e., putting the easier case first and avoiding a variable format string.
> @@ -1086,7 +1091,7 @@ static struct store *imap_open_store(struct imap_server_conf *srvc)
> int gai;
> char portstr[6];
>
> - snprintf(portstr, sizeof(portstr), "%hu", srvc->port);
> + snprintf(portstr, sizeof(portstr), "%hu", (unsigned short)srvc->port);
Why not
snprintf(portstr, sizeof(portstr), "%d", srvc->port);
?
Thanks for checking the code.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC/PATCH] imap-send: Code correctness flagged by clang
2010-08-07 21:04 ` Jonathan Nieder
@ 2010-08-07 22:53 ` Ævar Arnfjörð Bjarmason
2010-08-07 23:09 ` [PATCH maint] imap-send: Fix sprintf usage Jonathan Nieder
0 siblings, 1 reply; 5+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2010-08-07 22:53 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Mike McCormack, Benjamin Kramer
On Sat, Aug 7, 2010 at 21:04, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Ævar Arnfjörð Bjarmason wrote:
>
>> [Subject: imap-send: Code correctness flagged by clang]
>>
>> Clang 1.1 flagged the following issues in imap-send.c, this change
>> fixes the warnings by moving some code around:
>>
>> imap-send.c:548:27: warning: data argument not used by format string [-Wformat-extra-args]
>> cmd->tag, cmd->cmd, cmd->cb.dlen);
>> ^
>>
>> Here the sprintf format didn't use the cmd->cb.dlen argument if
>> cmd->cb.data was false. Change the code to use a if/else instead of a
>> two-level ternary to work it. This code was introduced with imap-send
>> itself in f2561fda.
>>
>> imap-send.c:1089:41: warning: conversion specifies type 'unsigned short' but the argument has type 'int' [-Wformat]
>> snprintf(portstr, sizeof(portstr), "%hu", srvc->port);
>> ~~^ ~~~~~~~~~~
>>
>> Here sprintf is being given an int with a %hu format. Cast the
>> srvc->port to unsigned short to work it. This code was introduced in
>> 94ad2437 to add IPv6 support.
>
> Nitpick: that this was found by clang is probably not the first thing
> a person trying to figure out what the patch does needs to know.
> Maybe:
>
> Subject: imap-send: Fix sprintf usage
>
> When composing a command for the imap server, imap-send
> uses a single nfsnprintf() invocation for brevity
> instead of dealing separately with the case when there
> is a message to be sent and the case when there isn’t.
> The unused argument in the second case, while valid,
> is confusing for static analyzers and human readers.
>
> v1.6.4-rc0~117 (imap-send: add support for IPv6, 2009-05-25)
> mistakenly used %hu as the format for an int “port”, by
> analogy with existing usage for the unsigned short
> “addr.sin_port”. Use %d instead.
>
> Noticed with clang.
That looks better.
>> +++ b/imap-send.c
>> @@ -543,9 +543,14 @@ static struct imap_cmd *v_issue_imap_cmd(struct imap_store *ctx,
>> while (imap->literal_pending)
>> get_cmd_result(ctx, NULL);
>>
>> - bufl = nfsnprintf(buf, sizeof(buf), cmd->cb.data ? CAP(LITERALPLUS) ?
>> - "%d %s{%d+}\r\n" : "%d %s{%d}\r\n" : "%d %s\r\n",
>> - cmd->tag, cmd->cmd, cmd->cb.dlen);
>> + if (cmd->cb.data) {
>> + bufl = nfsnprintf(buf, sizeof(buf),
>> + CAP(LITERALPLUS) ? "%d %s{%d+}\r\n" : "%d %s{%d}\r\n",
>> + cmd->tag, cmd->cmd, cmd->cb.dlen);
>> + } else {
>> + bufl = nfsnprintf(buf, sizeof(buf), "%d %s\r\n", cmd->tag, cmd->cmd);
>> + }
>> +
>
> Hmm, maybe this would be easier to read:
>
> if (!cmd->cb.data)
> bufl = nfsnprintf(buf, sizeof(buf), "%d %s\r\n", cmd->tag, cmd->cmd);
> else
> bufl = nfsnprintf(buf, sizeof(buf), "%d %s{%d%s}\r\n",
> cmd->tag, cmd->cmd, cmd->cb.dlen,
> CAP(LITERALPLUS) ? "+" : "");
>
> i.e., putting the easier case first and avoiding a variable format string.
Yeah, that version looks better.
>> @@ -1086,7 +1091,7 @@ static struct store *imap_open_store(struct imap_server_conf *srvc)
>> int gai;
>> char portstr[6];
>>
>> - snprintf(portstr, sizeof(portstr), "%hu", srvc->port);
>> + snprintf(portstr, sizeof(portstr), "%hu", (unsigned short)srvc->port);
>
> Why not
>
> snprintf(portstr, sizeof(portstr), "%d", srvc->port);
>
> ?
I wasn't sure whether it needed to be %hu for the purposes of the
snprintf() call. I.e. that the resulting contents of portstr might be
different on some systems.
Maybe they won't be, then we could just use %d.
Another alternative would be to change the definition of port from int
to unsigned short in the srvc struct.
> Thanks for checking the code.
Thanks for reviewing the patch.
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH maint] imap-send: Fix sprintf usage
2010-08-07 22:53 ` Ævar Arnfjörð Bjarmason
@ 2010-08-07 23:09 ` Jonathan Nieder
2010-08-07 23:25 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 5+ messages in thread
From: Jonathan Nieder @ 2010-08-07 23:09 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Mike McCormack, Benjamin Kramer, Junio C Hamano
From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
When composing a command for the imap server, imap-send uses a single
nfsnprintf() invocation for brevity instead of dealing separately with
the case when there is a message to be sent and the case when there
isn’t. The unused argument in the second case, while valid, is
confusing for static analyzers and human readers.
v1.6.4-rc0~117 (imap-send: add support for IPv6, 2009-05-25)
mistakenly used %hu as the format for an int “port”, by analogy with
existing usage for the unsigned short “addr.sin_port”. Use %d
instead.
Noticed with clang.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Ævar Arnfjörð Bjarmason wrote:
> I wasn't sure whether it needed to be %hu for the purposes of the
> snprintf() call. I.e. that the resulting contents of portstr might be
> different on some systems.
>
> Maybe they won't be, then we could just use %d.
It’s just a nonnegative integer. :)
Here’s the updated patch. Untested.
imap-send.c | 12 ++++++++----
1 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/imap-send.c b/imap-send.c
index 1a577a0..3a02e89 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -543,9 +543,13 @@ static struct imap_cmd *v_issue_imap_cmd(struct imap_store *ctx,
while (imap->literal_pending)
get_cmd_result(ctx, NULL);
- bufl = nfsnprintf(buf, sizeof(buf), cmd->cb.data ? CAP(LITERALPLUS) ?
- "%d %s{%d+}\r\n" : "%d %s{%d}\r\n" : "%d %s\r\n",
- cmd->tag, cmd->cmd, cmd->cb.dlen);
+ if (!cmd->cb.data)
+ bufl = nfsnprintf(buf, sizeof(buf), "%d %s\r\n", cmd->tag, cmd->cmd);
+ else
+ bufl = nfsnprintf(buf, sizeof(buf), "%d %s{%d%s}\r\n",
+ cmd->tag, cmd->cmd, cmd->cb.dlen,
+ CAP(LITERALPLUS) ? "+" : "");
+
if (Verbose) {
if (imap->num_in_progress)
printf("(%d in progress) ", imap->num_in_progress);
@@ -1086,7 +1090,7 @@ static struct store *imap_open_store(struct imap_server_conf *srvc)
int gai;
char portstr[6];
- snprintf(portstr, sizeof(portstr), "%hu", srvc->port);
+ snprintf(portstr, sizeof(portstr), "%d", srvc->port);
memset(&hints, 0, sizeof(hints));
hints.ai_socktype = SOCK_STREAM;
--
1.7.2.1.544.ga752d.dirty
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH maint] imap-send: Fix sprintf usage
2010-08-07 23:09 ` [PATCH maint] imap-send: Fix sprintf usage Jonathan Nieder
@ 2010-08-07 23:25 ` Ævar Arnfjörð Bjarmason
0 siblings, 0 replies; 5+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2010-08-07 23:25 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Mike McCormack, Benjamin Kramer, Junio C Hamano
Thanks for submitting this, especially since I was too lazy to do it
myself today. It looks good.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-08-07 23:25 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-08-07 12:12 [RFC/PATCH] imap-send: Code correctness flagged by clang Ævar Arnfjörð Bjarmason
2010-08-07 21:04 ` Jonathan Nieder
2010-08-07 22:53 ` Ævar Arnfjörð Bjarmason
2010-08-07 23:09 ` [PATCH maint] imap-send: Fix sprintf usage Jonathan Nieder
2010-08-07 23:25 ` Ævar Arnfjörð Bjarmason
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.