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

Christoph Hellwig
2021-06-15T08:10:45Z

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"

???