Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190227202202.21817-1-louis@kragniz.eu>
In-Reply-To: <20190227202202.21817-1-louis@kragniz.eu>
From:   Nick Desaulniers <ndesaulniers@google.com>
Date:   Wed, 27 Feb 2019 14:00:25 -0800
Message-ID: <CAKwvOd=P50WiPMzQtM91Keo2J1Cb=VmCnnAb6Ly_zRQhSaCW1A@mail.gmail.com>
Subject: Re: [PATCH] cifs: use correct format characters
To:     Louis Taylor <louis@kragniz.eu>
Cc:     sfrench@samba.org, linux-cifs@vger.kernel.org,
        samba-technical@lists.samba.org,
        LKML <linux-kernel@vger.kernel.org>,
        clang-built-linux@googlegroups.com,
        Jon Flatley <jflat@chromium.org>,
        =?UTF-8?Q?Matthias_M=C3=A4nnich?= <maennich@google.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Feb 27, 2019 at 12:23 PM Louis Taylor <louis@kragniz.eu> wrote:
>
> When compiling with -Wformat, clang emits the following warnings:
>
> fs/cifs/smb1ops.c:312:20: warning: format specifies type 'unsigned
> short' but the argument has type 'unsigned int' [-Wformat]
>                          tgt_total_cnt, total_in_tgt);
>                                         ^~~~~~~~~~~~

Thanks for the patch! This one fails a small sanity check and needs a revision:

So 1 warning in fs/cifs/smb1ops.c, but 2 fixes?

>
> fs/cifs/cifs_dfs_ref.c:289:4: warning: format specifies type 'short'
> but the argument has type 'int' [-Wformat]
>                  ref->flags, ref->server_type);
>                  ^~~~~~~~~~
>
> fs/cifs/cifs_dfs_ref.c:289:16: warning: format specifies type 'short'
> but the argument has type 'int' [-Wformat]
>                  ref->flags, ref->server_type);
>                              ^~~~~~~~~~~~~~~~
>
> fs/cifs/cifs_dfs_ref.c:291:4: warning: format specifies type 'short'
> but the argument has type 'int' [-Wformat]
>                  ref->ref_flag, ref->path_consumed);
>                  ^~~~~~~~~~~~~
>
> fs/cifs/cifs_dfs_ref.c:291:19: warning: format specifies type 'short'
> but the argument has type 'int' [-Wformat]
>                  ref->ref_flag, ref->path_consumed);
>                                 ^~~~~~~~~~~~~~~~~~
> The types of these arguments are unconditionally defined, so this patch
> updates the format character to the correct ones for ints and unsigned
> ints.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/378
> Signed-off-by: Louis Taylor <louis@kragniz.eu>
> ---
>  fs/cifs/cifs_dfs_ref.c | 4 ++--
>  fs/cifs/smb1ops.c      | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/fs/cifs/cifs_dfs_ref.c b/fs/cifs/cifs_dfs_ref.c
> index d9b99abe1243..5d83c924cc47 100644
> --- a/fs/cifs/cifs_dfs_ref.c
> +++ b/fs/cifs/cifs_dfs_ref.c
> @@ -285,9 +285,9 @@ static void dump_referral(const struct dfs_info3_param *ref)
>  {
>         cifs_dbg(FYI, "DFS: ref path: %s\n", ref->path_name);
>         cifs_dbg(FYI, "DFS: node path: %s\n", ref->node_name);
> -       cifs_dbg(FYI, "DFS: fl: %hd, srv_type: %hd\n",
> +       cifs_dbg(FYI, "DFS: fl: %d, srv_type: %d\n",
>                  ref->flags, ref->server_type);
> -       cifs_dbg(FYI, "DFS: ref_flags: %hd, path_consumed: %hd\n",
> +       cifs_dbg(FYI, "DFS: ref_flags: %d, path_consumed: %d\n",
>                  ref->ref_flag, ref->path_consumed);
>  }
>
> diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
> index 32a6c020478f..52e1ae6744de 100644
> --- a/fs/cifs/smb1ops.c
> +++ b/fs/cifs/smb1ops.c
> @@ -300,7 +300,7 @@ coalesce_t2(char *second_buf, struct smb_hdr *target_hdr)
>         tgt_total_cnt = get_unaligned_le16(&pSMBt->t2_rsp.TotalDataCount);
>
>         if (tgt_total_cnt != src_total_cnt)
> -               cifs_dbg(FYI, "total data count of primary and secondary t2 differ source=%hu target=%hu\n",
> +               cifs_dbg(FYI, "total data count of primary and secondary t2 differ source=%hu target=%u\n",
>                          src_total_cnt, tgt_total_cnt);

This one is not correct; while you'd be forgiven for confusing the
case here `tgt_total_cnt` with `total_in_tgt` below, the above hunk is
not correct.  `tgt_total_cnt` is an instance of a `__u16`.  Please
resend with this hunk dropped.  The other hunks look correct.

>
>         total_in_tgt = get_unaligned_le16(&pSMBt->t2_rsp.DataCount);
> @@ -308,7 +308,7 @@ coalesce_t2(char *second_buf, struct smb_hdr *target_hdr)
>         remaining = tgt_total_cnt - total_in_tgt;
>
>         if (remaining < 0) {
> -               cifs_dbg(FYI, "Server sent too much data. tgt_total_cnt=%hu total_in_tgt=%hu\n",
> +               cifs_dbg(FYI, "Server sent too much data. tgt_total_cnt=%hu total_in_tgt=%u\n",
>                          tgt_total_cnt, total_in_tgt);
>                 return -EPROTO;
>         }
> --
> 2.20.1
>


-- 
Thanks,
~Nick Desaulniers