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

[Christoph Hellwig <hch@infradead.org>]

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?

> +#define DATA_STREAM	1
> +#define DIR_STREAM	2

Should this use a named enum to document the usage a bit better?

> +#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.

Also can you please decided if this is kcifsd or ksmbd?  Using both
names is rather confusing.

> +#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.

> +#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?

> +/* @FIXME */
> +#include "ksmbd_server.h"

???