On 17 Apr 2024, at 8:40, Dan Carpenter wrote:
> On Wed, Apr 17, 2024 at 08:00:04AM -0400, Benjamin Coddington wrote:
>> On 15 Apr 2024, at 4:08, Dan Carpenter wrote:
>>
>>> [ Why is Smatch only complaining now, 2 years later??? It is a mystery.
>>> -dan ]
>>>
>>> Hello Benjamin Coddington,
>>
>> Hi Dan!
>>
>>> Commit c3ed222745d9 ("NFSv4: Fix free of uninitialized nfs4_label on
>>> referral lookup.") from May 14, 2022 (linux-next), leads to the
>>> following Smatch static checker warning:
>>>
>>> fs/nfs/nfs4state.c:2138 nfs4_try_migration()
>>> warn: missing error code here? 'nfs_alloc_fattr()' failed. 'result' = '0'
>>>
>>> fs/nfs/nfs4state.c
>>> 2115 static int nfs4_try_migration(struct nfs_server *server, const struct cred *cred)
>>> 2116 {
>>> 2117 struct nfs_client *clp = server->nfs_client;
>>> 2118 struct nfs4_fs_locations *locations = NULL;
>>> 2119 struct inode *inode;
>>> 2120 struct page *page;
>>> 2121 int status, result;
>>> 2122
>>> 2123 dprintk("--> %s: FSID %llx:%llx on \"%s\"\n", __func__,
>>> 2124 (unsigned long long)server->fsid.major,
>>> 2125 (unsigned long long)server->fsid.minor,
>>> 2126 clp->cl_hostname);
>>> 2127
>>> 2128 result = 0;
>>> ^^^^^^^^^^^
>>>
>>> 2129 page = alloc_page(GFP_KERNEL);
>>> 2130 locations = kmalloc(sizeof(struct nfs4_fs_locations), GFP_KERNEL);
>>> 2131 if (page == NULL || locations == NULL) {
>>> 2132 dprintk("<-- %s: no memory\n", __func__);
>>> 2133 goto out;
>>> ^^^^^^^^
>>> Success.
>>>
>>> 2134 }
>>> 2135 locations->fattr = nfs_alloc_fattr();
>>> 2136 if (locations->fattr == NULL) {
>>> 2137 dprintk("<-- %s: no memory\n", __func__);
>>> --> 2138 goto out;
>>> ^^^^^^^^^
>>> Here too.
>>
>> My patch was following the precedent set by c9fdeb280b8cc. I believe the
>> idea is that the function can fail without an error and the client will
>> retry the next time the server says -NFS4ERR_MOVED.
>>
>> Is there a way to appease smatch here? I don't have a lot of smatch
>> smarts.
>
> Generally, I tell people to just ignore it. Anyone with questions can
> look up this email thread.
>
> But if you really wanted to silence it, Smatch counts it as intentional
> if the "result = 0;" is within five lines of the goto out.
Good to know! In this case, I think the maintainers would show annoyance
with that sort of patch. A comment here about the successful return code on
an allocation failure would have avoided this, and I probably should have
recognized this patch might create an issue and inserted one. Thanks for
the report.
Ben
