[PATCH 1/1 linux-next] fs/coda/upcall.c: remove UPARG flow control macro

[PATCH 1/1 linux-next] fs/coda/upcall.c: remove UPARG flow control macro

[Fabian Frederick]

Move UPARG code to alloc_upcall() and test errors/return in callsites.
This patch removes flow control in macros which must be avoided.
(See Documentation/CodingStyle)

Signed-off-by: Fabian Frederick <fabf@skynet.be>
---
This is untested.

 fs/coda/upcall.c | 107 +++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 69 insertions(+), 38 deletions(-)

diff --git a/fs/coda/upcall.c b/fs/coda/upcall.c
index 9b1ffaa..7524f630 100644
--- a/fs/coda/upcall.c
+++ b/fs/coda/upcall.c
@@ -41,30 +41,24 @@
 static int coda_upcall(struct venus_comm *vc, int inSize, int *outSize,
 		       union inputArgs *buffer);
 
-static void *alloc_upcall(int opcode, int size)
+static int alloc_upcall(int opcode, int insize, int *outsize,
+			union inputArgs **inp, union outputArgs **outp)
 {
-	union inputArgs *inp;
+	CODA_ALLOC(*inp, union inputArgs *, insize);
+	if (!*inp)
+		return -ENOMEM;
 
-	CODA_ALLOC(inp, union inputArgs *, size);
-        if (!inp)
-		return ERR_PTR(-ENOMEM);
+	(*inp)->ih.opcode = opcode;
+	(*inp)->ih.pid = task_pid_nr_ns(current, &init_pid_ns);
+	(*inp)->ih.pgid = task_pgrp_nr_ns(current, &init_pid_ns);
+	(*inp)->ih.uid = from_kuid(&init_user_ns, current_fsuid());
 
-        inp->ih.opcode = opcode;
-	inp->ih.pid = task_pid_nr_ns(current, &init_pid_ns);
-	inp->ih.pgid = task_pgrp_nr_ns(current, &init_pid_ns);
-	inp->ih.uid = from_kuid(&init_user_ns, current_fsuid());
+	*outp = (union outputArgs *)inp;
+	*outsize = insize;
 
-	return (void*)inp;
+	return 0;
 }
 
-#define UPARG(op)\
-do {\
-	inp = (union inputArgs *)alloc_upcall(op, insize); \
-        if (IS_ERR(inp)) { return PTR_ERR(inp); }\
-        outp = (union outputArgs *)(inp); \
-        outsize = insize; \
-} while (0)
-
 #define INSIZE(tag) sizeof(struct coda_ ## tag ## _in)
 #define OUTSIZE(tag) sizeof(struct coda_ ## tag ## _out)
 #define SIZE(tag)  max_t(unsigned int, INSIZE(tag), OUTSIZE(tag))
@@ -78,7 +72,9 @@ int venus_rootfid(struct super_block *sb, struct CodaFid *fidp)
         int insize, outsize, error;
 
         insize = SIZE(root);
-        UPARG(CODA_ROOT);
+	error = alloc_upcall(CODA_ROOT, insize, &outsize, &inp, &outp);
+	if (error)
+		return error;
 
 	error = coda_upcall(coda_vcp(sb), insize, &outsize, inp);
 	if (!error)
@@ -96,7 +92,10 @@ int venus_getattr(struct super_block *sb, struct CodaFid *fid,
         int insize, outsize, error;
 
         insize = SIZE(getattr); 
-	UPARG(CODA_GETATTR);
+	error = alloc_upcall(CODA_GETATTR, insize, &outsize, &inp, &outp);
+	if (error)
+		return error;
+
         inp->coda_getattr.VFid = *fid;
 
 	error = coda_upcall(coda_vcp(sb), insize, &outsize, inp);
@@ -115,7 +114,9 @@ int venus_setattr(struct super_block *sb, struct CodaFid *fid,
         int insize, outsize, error;
 	
 	insize = SIZE(setattr);
-	UPARG(CODA_SETATTR);
+	error = alloc_upcall(CODA_SETATTR, insize, &outsize, &inp, &outp);
+	if (error)
+		return error;
 
         inp->coda_setattr.VFid = *fid;
 	inp->coda_setattr.attr = *vattr;
@@ -137,7 +138,9 @@ int venus_lookup(struct super_block *sb, struct CodaFid *fid,
 
 	offset = INSIZE(lookup);
         insize = max_t(unsigned int, offset + length +1, OUTSIZE(lookup));
-	UPARG(CODA_LOOKUP);
+	error = alloc_upcall(CODA_LOOKUP, insize, &outsize, &inp, &outp);
+	if (error)
+		return error;
 
         inp->coda_lookup.VFid = *fid;
 	inp->coda_lookup.name = offset;
@@ -164,8 +167,10 @@ int venus_close(struct super_block *sb, struct CodaFid *fid, int flags,
 	int insize, outsize, error;
 	
 	insize = SIZE(release);
-	UPARG(CODA_CLOSE);
-	
+	error = alloc_upcall(CODA_CLOSE, insize, &outsize, &inp, &outp);
+	if (error)
+		return error;
+
 	inp->ih.uid = from_kuid(&init_user_ns, uid);
         inp->coda_close.VFid = *fid;
         inp->coda_close.flags = flags;
@@ -184,7 +189,9 @@ int venus_open(struct super_block *sb, struct CodaFid *fid,
         int insize, outsize, error;
        
 	insize = SIZE(open_by_fd);
-	UPARG(CODA_OPEN_BY_FD);
+	error = alloc_upcall(CODA_OPEN_BY_FD, insize, &outsize, &inp, &outp);
+	if (error)
+		return error;
 
 	inp->coda_open_by_fd.VFid = *fid;
 	inp->coda_open_by_fd.flags = flags;
@@ -208,7 +215,9 @@ int venus_mkdir(struct super_block *sb, struct CodaFid *dirfid,
 
 	offset = INSIZE(mkdir);
 	insize = max_t(unsigned int, offset + length + 1, OUTSIZE(mkdir));
-	UPARG(CODA_MKDIR);
+	error = alloc_upcall(CODA_MKDIR, insize, &outsize, &inp, &outp);
+	if (error)
+		return error;
 
         inp->coda_mkdir.VFid = *dirfid;
         inp->coda_mkdir.attr = *attrs;
@@ -241,7 +250,9 @@ int venus_rename(struct super_block *sb, struct CodaFid *old_fid,
 	offset = INSIZE(rename);
 	insize = max_t(unsigned int, offset + new_length + old_length + 8,
 		     OUTSIZE(rename)); 
- 	UPARG(CODA_RENAME);
+	error = alloc_upcall(CODA_RENAME, insize, &outsize, &inp, &outp);
+	if (error)
+		return error;
 
         inp->coda_rename.sourceFid = *old_fid;
         inp->coda_rename.destFid =  *new_fid;
@@ -276,7 +287,9 @@ int venus_create(struct super_block *sb, struct CodaFid *dirfid,
 
         offset = INSIZE(create);
 	insize = max_t(unsigned int, offset + length + 1, OUTSIZE(create));
-	UPARG(CODA_CREATE);
+	error = alloc_upcall(CODA_CREATE, insize, &outsize, &inp, &outp);
+	if (error)
+		return error;
 
         inp->coda_create.VFid = *dirfid;
         inp->coda_create.attr.va_mode = mode;
@@ -308,7 +321,9 @@ int venus_rmdir(struct super_block *sb, struct CodaFid *dirfid,
 
         offset = INSIZE(rmdir);
 	insize = max_t(unsigned int, offset + length + 1, OUTSIZE(rmdir));
-	UPARG(CODA_RMDIR);
+	error = alloc_upcall(CODA_RMDIR, insize, &outsize, &inp, &outp);
+	if (error)
+		return error;
 
         inp->coda_rmdir.VFid = *dirfid;
         inp->coda_rmdir.name = offset;
@@ -330,7 +345,9 @@ int venus_remove(struct super_block *sb, struct CodaFid *dirfid,
 
         offset = INSIZE(remove);
 	insize = max_t(unsigned int, offset + length + 1, OUTSIZE(remove));
-	UPARG(CODA_REMOVE);
+	error = alloc_upcall(CODA_REMOVE, insize, &outsize, &inp, &outp);
+	if (error)
+		return error;
 
         inp->coda_remove.VFid = *dirfid;
         inp->coda_remove.name = offset;
@@ -354,7 +371,9 @@ int venus_readlink(struct super_block *sb, struct CodaFid *fid,
         
 	insize = max_t(unsigned int,
 		     INSIZE(readlink), OUTSIZE(readlink)+ *length + 1);
-	UPARG(CODA_READLINK);
+	error = alloc_upcall(CODA_READLINK, insize, &outsize, &inp, &outp);
+	if (error)
+		return error;
 
         inp->coda_readlink.VFid = *fid;
 
@@ -385,7 +404,9 @@ int venus_link(struct super_block *sb, struct CodaFid *fid,
 
 	offset = INSIZE(link);
 	insize = max_t(unsigned int, offset  + len + 1, OUTSIZE(link));
-        UPARG(CODA_LINK);
+	error = alloc_upcall(CODA_LINK, insize, &outsize, &inp, &outp);
+	if (error)
+		return error;
 
         inp->coda_link.sourceFid = *fid;
         inp->coda_link.destFid = *dirfid;
@@ -412,8 +433,10 @@ int venus_symlink(struct super_block *sb, struct CodaFid *fid,
 
         offset = INSIZE(symlink);
 	insize = max_t(unsigned int, offset + len + symlen + 8, OUTSIZE(symlink));
-	UPARG(CODA_SYMLINK);
-        
+	error = alloc_upcall(CODA_SYMLINK, insize, &outsize, &inp, &outp);
+	if (error)
+		return error;
+
         /*        inp->coda_symlink.attr = *tva; XXXXXX */ 
         inp->coda_symlink.VFid = *fid;
 
@@ -443,7 +466,9 @@ int venus_fsync(struct super_block *sb, struct CodaFid *fid)
 	int insize, outsize, error;
 	
 	insize=SIZE(fsync);
-	UPARG(CODA_FSYNC);
+	error = alloc_upcall(CODA_FSYNC, insize, &outsize, &inp, &outp);
+	if (error)
+		return error;
 
 	inp->coda_fsync.VFid = *fid;
 	error = coda_upcall(coda_vcp(sb), sizeof(union inputArgs),
@@ -460,7 +485,9 @@ int venus_access(struct super_block *sb, struct CodaFid *fid, int mask)
 	int insize, outsize, error;
 
 	insize = SIZE(access);
-	UPARG(CODA_ACCESS);
+	error = alloc_upcall(CODA_ACCESS, insize, &outsize, &inp, &outp);
+	if (error)
+		return error;
 
         inp->coda_access.VFid = *fid;
         inp->coda_access.flags = mask;
@@ -481,7 +508,9 @@ int venus_pioctl(struct super_block *sb, struct CodaFid *fid,
 	int iocsize;
 
 	insize = VC_MAXMSGSIZE;
-	UPARG(CODA_IOCTL);
+	error = alloc_upcall(CODA_IOCTL, insize, &outsize, &inp, &outp);
+	if (error)
+		return error;
 
         /* build packet for Venus */
         if (data->vi.in_size > VC_MAXDATASIZE) {
@@ -554,7 +583,9 @@ int venus_statfs(struct dentry *dentry, struct kstatfs *sfs)
         int insize, outsize, error;
         
 	insize = max_t(unsigned int, INSIZE(statfs), OUTSIZE(statfs));
-	UPARG(CODA_STATFS);
+	error = alloc_upcall(CODA_STATFS, insize, &outsize, &inp, &outp);
+	if (error)
+		return error;
 
 	error = coda_upcall(coda_vcp(dentry->d_sb), insize, &outsize, inp);
 	if (!error) {
-- 
1.9.1

Re: [PATCH 1/1 linux-next] fs/coda/upcall.c: remove UPARG flow control macro

[Jan Harkes]

On Mon, Apr 27, 2015 at 07:23:06PM +0200, Fabian Frederick wrote:
> Move UPARG code to alloc_upcall() and test errors/return in callsites.
> This patch removes flow control in macros which must be avoided.
> (See Documentation/CodingStyle)

At first glance this is not a correct patch.

UPARG allocates a buffer that is large enough to hold both the upcall
message going to userspace as well as the incoming reply message. At
first glance this patch only seems to allocate insize. It may still be
doing the right thing because insize is defined at the call level as
MAX(sizeof(inarg), sizeof(outarg)). This it looks like quite a few
changes for an untested patch and not enough for an actual cleanup of
the related code.

Jan

> ---
> This is untested.
> 
>  fs/coda/upcall.c | 107 +++++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 69 insertions(+), 38 deletions(-)
> 
> diff --git a/fs/coda/upcall.c b/fs/coda/upcall.c
> index 9b1ffaa..7524f630 100644
> --- a/fs/coda/upcall.c
> +++ b/fs/coda/upcall.c
> @@ -41,30 +41,24 @@
>  static int coda_upcall(struct venus_comm *vc, int inSize, int *outSize,
>  		       union inputArgs *buffer);
>  
> -static void *alloc_upcall(int opcode, int size)
> +static int alloc_upcall(int opcode, int insize, int *outsize,
> +			union inputArgs **inp, union outputArgs **outp)
>  {
> -	union inputArgs *inp;
> +	CODA_ALLOC(*inp, union inputArgs *, insize);
> +	if (!*inp)
> +		return -ENOMEM;
>  
> -	CODA_ALLOC(inp, union inputArgs *, size);
> -        if (!inp)
> -		return ERR_PTR(-ENOMEM);
...

Re: [PATCH 1/1 linux-next] fs/coda/upcall.c: remove UPARG flow control macro

[Fabian Frederick]

> On 27 April 2015 at 20:37 Jan Harkes <jaharkes@cs.cmu.edu> wrote:
>
>
> On Mon, Apr 27, 2015 at 07:23:06PM +0200, Fabian Frederick wrote:
> > Move UPARG code to alloc_upcall() and test errors/return in callsites.
> > This patch removes flow control in macros which must be avoided.
> > (See Documentation/CodingStyle)
>
> At first glance this is not a correct patch.
>
> UPARG allocates a buffer that is large enough to hold both the upcall
> message going to userspace as well as the incoming reply message. At
> first glance this patch only seems to allocate insize. It may still be
> doing the right thing because insize is defined at the call level as
> MAX(sizeof(inarg), sizeof(outarg)). This it looks like quite a few
> changes for an untested patch and not enough for an actual cleanup of
> the related code.
>
> Jan

Sorry Jan but I don't see any problem in that patch.
outarg is processed the same way:

New alloc_upcall(**inp, **outp):
*outp = (union outputArgs *)inp;
*outsize = insize;

UPARG macro:
outp = (union outputArgs *)(inp); \
outsize = insize; \

Regards,
Fabian


>
> > ---
> > This is untested.
> >
> >  fs/coda/upcall.c | 107
> >+++++++++++++++++++++++++++++++++++--------------------
> >  1 file changed, 69 insertions(+), 38 deletions(-)
> >
> > diff --git a/fs/coda/upcall.c b/fs/coda/upcall.c
> > index 9b1ffaa..7524f630 100644
> > --- a/fs/coda/upcall.c
> > +++ b/fs/coda/upcall.c
> > @@ -41,30 +41,24 @@
> >  static int coda_upcall(struct venus_comm *vc, int inSize, int *outSize,
> >                    union inputArgs *buffer);
> > 
> > -static void *alloc_upcall(int opcode, int size)
> > +static int alloc_upcall(int opcode, int insize, int *outsize,
> > +                   union inputArgs **inp, union outputArgs **outp)
> >  {
> > -   union inputArgs *inp;
> > +   CODA_ALLOC(*inp, union inputArgs *, insize);
> > +   if (!*inp)
> > +           return -ENOMEM;
> > 
> > -   CODA_ALLOC(inp, union inputArgs *, size);
> > -        if (!inp)
> > -           return ERR_PTR(-ENOMEM);
> ...