RE: [PATCH v4 02/10] cifsd: add server handler

[Namjae Jeon]

Hi Christoph,
 
> On Wed, Jun 02, 2021 at 12:48:39PM +0900, Namjae Jeon wrote:
> > +/* @FIXME clean up this code */
> 
> Hmm, should that be in a submitted codebase?
No, Need to remove it:)

> 
> > +#define DATA_STREAM	1
> > +#define DIR_STREAM	2
> 
> Should this use a named enum to document the usage a bit better?
Yes, I will do that.

> 
> > +#ifndef ksmbd_pr_fmt
> > +#ifdef SUBMOD_NAME
> > +#define ksmbd_pr_fmt(fmt)	"ksmbd: " SUBMOD_NAME ": " fmt
> > +#else
> > +#define ksmbd_pr_fmt(fmt)	"ksmbd: " fmt
> > +#endif
> > +#endif
> 
> Why not use the pr_fmt built into pr_*?  With that all the message wrappers except for the _dbg one
> can go away.
Right. Will fix it.
> 
> Also can you please decided if this is kcifsd or ksmbd?  Using both names is rather confusing.
Ah, Are you saying patch subject prefix and document ? there is no use of kcifsd in source code.
If yes, I will replace it with ksmbd in there.
> 
> > +#ifndef ____ksmbd_align
> > +#define ____ksmbd_align		__aligned(4)
> > +#endif
> 
> No need for the ifndef and all the _ prefixes.  More importantly from a quick look it seems like none
> of the structures really needs the attribute anyway.
Okay.
> 
> > +#define KSMBD_SHARE_CONFIG_PATH(s)				\
> > +	({							\
> > +		char *p = (s)->____payload;			\
> > +		if ((s)->veto_list_sz)				\
> > +			p += (s)->veto_list_sz + 1;		\
> > +		p;						\
> > +	 })
> 
> Why no inline function?
Okay, will change it to inline function.
> 
> > +/* @FIXME */
> > +#include "ksmbd_server.h"
> 
> ???
Will remove it.

Thanks for your review!