[LU-3204] clean up SET_BUT_UNUSED

• Status: Resolved
• Project: Lustre
• Fix Version/s: Lustre 2.5.0
• Type: Bug
• Priority: Minor
• Reporter: Ned Bass
• Assignee: Dmitry Eremin (Inactive)
• Resolution: Fixed
• Labels: mn4
• Severity: 3

Description

The SET_BUT_UNUSED macro seems to exist purely to silence compiler warnings:

#define SET_BUT_UNUSED(a) do { } while(sizeof(a) - sizeof(a))

The hapless code reviewer cannot help but wonder "why not just fix the code to get rid of the warning?". Fixing the handful of places that use this macro would be a small but simple step toward making Lustre code more accessible.

In some cases, particularly with functions built in kernel and user-space, the SET_BUT_UNUSED usage seems to be a matter of style preference. That is, the original intent seemed to be to avoid #ifdef's around declarations of variables not used in user space. If we want to uphold this convention, the practice at least deserves an explanatory comment. I would argue that the code is already littered with #ifdef's, so one more around a variable declaration is less detrimental to readability than SET_BUT_UNSED is.

In other cases, SET_BUT_UNUSED is used rather confusingly to let some unfinished bit of code build cleanly. For example:

lustre/llite/dcache.c:ll_revalidate_it()

462                 if (it->it_flags & FMODE_WRITE) {                               
463                         och_p = &lli->lli_mds_write_och;                        
464                         och_usecount = &lli->lli_open_fd_write_count;           
465                 } else if (it->it_flags & FMODE_EXEC) {                         
466                         och_p = &lli->lli_mds_exec_och;                         
467                         och_usecount = &lli->lli_open_fd_exec_count;            
468                 } else {                                                        
469                         och_p = &lli->lli_mds_read_och;                         
470                         och_usecount = &lli->lli_open_fd_read_count;            
471                 }                                                               
472                 /* Check for the proper lock. */                                
473                 ibits = MDS_INODELOCK_LOOKUP;                                   
474                 if (!ll_have_md_lock(inode, &ibits, LCK_MINMODE))               
475                         goto do_lock;                                           
476                 mutex_lock(&lli->lli_och_mutex);                                
477                 if (*och_p) { /* Everything is open already, do nothing */      
478                         /*(*och_usecount)++;  Do not let them steal our open    
479                           handle from under us */                               
480                         SET_BUT_UNUSED(och_usecount);                           
481                         /* XXX The code above was my original idea, but in case 
482                            we have the handle, but we cannot use it due to later
483                            checks (e.g. O_CREAT|O_EXCL flags set), nobody       
484                            would decrement counter increased here. So we just   
485                            hope the lock won't be invalidated in between. But   
486                            if it would be, we'll reopen the open request to     
487                            MDS later during file open path */                   
488                         mutex_unlock(&lli->lli_och_mutex);                      
489                         RETURN(1);                                              
490                 } else {                                                        
491                         mutex_unlock(&lli->lli_och_mutex);                      
492                 }                                                               
493         }

Comments

[Peter Jones]

Dmitry

Could you please look into this ticket?

Thanks

Peter