Æ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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html