Messages in this thread | | | Date | Tue, 15 Jun 2021 09:10:06 +0100 | From | Christoph Hellwig <> | Subject | Re: [PATCH v4 02/10] cifsd: add server handler |
| |
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"
???
| |