Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 15 Jun 2021 09:10:06 +0100
From:   Christoph Hellwig <hch@infradead.org>
To:     Namjae Jeon <namjae.jeon@samsung.com>
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,
        hch@infradead.org, 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
Message-ID: <YMhgXsAez9jmeUkW@infradead.org>
References: <20210602034847.5371-1-namjae.jeon@samsung.com>
 <CGME20210602035815epcas1p18f19c96ea3d299f97a90d818b83a3c85@epcas1p1.samsung.com>
 <20210602034847.5371-3-namjae.jeon@samsung.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210602034847.5371-3-namjae.jeon@samsung.com>
Precedence: bulk

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"

???