[LU-5411] cfs_hlist_for_each_entry() not working as intended

• Type: Bug
• Resolution: Duplicate
• Priority: Minor
• Severity: 3
• Assignee: WC Triage
• Reporter: Frank Zago (Inactive)

Description

When HAVE_HLIST_FOR_EACH_3ARG is defined, cfs_hlist_for_each_entry_safe (and similarly cfs_hlist_for_each_entry) are defined as this:

        pos = NULL; hlist_for_each_entry_safe(tpos, n, head, member)

However if we have some code like:

            for (i = 0; i < REMOTE_PERM_HASHSIZE; i++)
                    cfs_hlist_for_each_entry_safe(lrp, node, next, hash + i,
                                                  lrp_list)
                            free_ll_remote_perm(lrp);

we end up actually doing:

  
            for (i = 0; i < REMOTE_PERM_HASHSIZE; i++)
                pos = NULL;
            hlist_for_each_entry_safe(lrp, node, next, hash + i, lrp_list)
                            free_ll_remote_perm(lrp);

which is probably not what was intended.

pos = NULL can be safely removed since it's an internal cursor. However that will generate some "unused" warnings on platforms where HAVE_HLIST_FOR_EACH_3ARG is defined.

The other option is to add braces around the call, such as:

  
            for (i = 0; i < REMOTE_PERM_HASHSIZE; i++) {
                    cfs_hlist_for_each_entry_safe(lrp, node, next, hash + i,
                                                  lrp_list)
                            free_ll_remote_perm(lrp);
            }

However that won't prevent a similar bug from being introduced in the future.

Issue Links

• is related to: LU-3963 cleanup libcfs wrappers

Comments

[James A Simmons]

Patch http://review.whamcloud.com/#/c/11005 which has been merged has resolved these issues. This ticket can be closed.

[James A Simmons]

Been looking at how linux has this to make it compatible. It appears they have a macro to handle this across different compilers. We should use __maybe_unused instead.

[Frank Zago (Inactive)]

It's supported by clang too, and probably (Intel's) icc.

[James A Simmons]

Very gcc specific. Perhaps we could do a pragma equivalent.

[Frank Zago (Inactive)]

A bit ugly, but the unused attribute could be used:

 cfs_hlist_node_t *pos __attribute__ ((unused));

[James A Simmons]

Your bracket idea is not working.

/usr/src/kernels/3.12.22/arch/x86/Makefile:113: CONFIG_X86_X32 enabled but no binutils support
CC [M] /chexport/users/jsimmons/lustre-2.6.50/lustre/llite/remote_perm.o
cc1: warnings being treated as errors
/chexport/users/jsimmons/lustre-2.6.50/lustre/llite/remote_perm.c: In function 'free_rmtperm_hash':
/chexport/users/jsimmons/lustre-2.6.50/lustre/llite/remote_perm.c:102: error: unused variable 'node'

I think it will need a more complex solution. Thinking...

[James A Simmons]

Discovering this during your SLES12 testing Be aware some other patches are floating around to clean up hlist handling that might conflict with what you want to do. Looking at my patches your cleanup idea only conflicts with 2 patches,

http://review.whamcloud.com/#/c/10387
http://review.whamcloud.com/#/c/11005

Patch 103887 needs to be rebased but I was waiting until patch 11005 lands first. Perhaps we can roll your changes into the 10387 patches after 11005 lands. My other patches for LU-3963 don't look like they will conflict. Outside those patches the files needed to be updated with your change are:

lustre/obdclass/capa.c
luste/llite/remote_perm.c
lustre/ptlrpc/gss/gss_pipefs.c
luste/ptlrpc/gss/gss_keyring.c

We will need to coordinate our work in some way.