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!
