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

From: "Namjae Jeon" <namjae.jeon@samsung.com>
To: "'Christoph Hellwig'" <hch@infradead.org>
Cc: <linux-fsdevel@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<linux-cifs@vger.kernel.org>, <smfrench@gmail.com>,
	<stfrench@microsoft.com>, <willy@infradead.org>,
	<aurelien.aptel@gmail.com>,
	<linux-cifsd-devel@lists.sourceforge.net>,
	<senozhatsky@chromium.org>, <sandeen@sandeen.net>,
	<aaptel@suse.com>, <viro@zeniv.linux.org.uk>,
	<ronniesahlberg@gmail.com>, <hch@lst.de>,
	<dan.carpenter@oracle.com>
Subject: RE: [PATCH v4 02/10] cifsd: add server handler
Date: Wed, 16 Jun 2021 15:49:09 +0900	[thread overview]
Message-ID: <009d01d7627b$b59aa7b0$20cff710$@samsung.com> (raw)
In-Reply-To: <YMhgXsAez9jmeUkW@infradead.org>

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!