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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 16 Jun 2021 15:49:09 +0900
From:   "Namjae Jeon" <namjae.jeon@...sung.com>
To:     "'Christoph Hellwig'" <hch@...radead.org>
Cc:     <linux-fsdevel@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-cifs@...r.kernel.org>, <smfrench@...il.com>,
        <stfrench@...rosoft.com>, <willy@...radead.org>,
        <aurelien.aptel@...il.com>,
        <linux-cifsd-devel@...ts.sourceforge.net>,
        <senozhatsky@...omium.org>, <sandeen@...deen.net>,
        <aaptel@...e.com>, <viro@...iv.linux.org.uk>,
        <ronniesahlberg@...il.com>, <hch@....de>,
        <dan.carpenter@...cle.com>
Subject: RE: [PATCH v4 02/10] cifsd: add server handler

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!