Re: [XenPPC] [PATCH] Fix up potential memory leaks introduced by xencomm patch

[Jerone Young <jyoung5@xxxxxxxxxx>]

On Thu, 2007-02-08 at 07:18 -0500, Jimi Xenidis wrote:
> mostly Linux Kernel style NITS
> 
> On Feb 7, 2007, at 4:26 PM, Jerone Young wrote:
> 
> > With some of the logic change from the Xencomm patch. In a few
> > hypercalls introduces a situation where you can potentially have  
> > memory
> > leaks if something fails. This patch address these issues.
> >
> > Signed-off-by: Jerone Young <jyoung5@xxxxxxxxxx>
> 
> > diff -r 37ea4cf1281a arch/powerpc/platforms/xen/hcall.c
> > --- a/arch/powerpc/platforms/xen/hcall.c    Tue Feb 06 17:10:20 2007  
> > -0500
> > +++ b/arch/powerpc/platforms/xen/hcall.c    Wed Feb 07 14:50:05 2007  
> > -0600
> > @@ -203,11 +203,16 @@ int HYPERVISOR_sched_op(int cmd, void *a
> >     desc = xencomm_map_no_alloc(arg, argsize);
> > -   if (desc == NULL)
> > -           return -EINVAL;
> > -
> > -   rc = plpar_hcall_norets(XEN_MARK(__HYPERVISOR_sched_op),
> > -                           cmd, desc);
> > +   if (desc)
> > +   {
> 
> brace on same line
> 
> > +           rc = plpar_hcall_norets(XEN_MARK(__HYPERVISOR_sched_op),
> > +                                   cmd, desc);
> > +           xencomm_free(desc);
> > +   }
> > +   else
> > +   {
> 
> no braces for single line, in fact avoid the single line by pre- 
> assigning the rc, the compiler will do the right optimization anyway.
> 
> > +           rc = -EINVAL;
> 
> I thought we agreed that xencomm_map_* failures would be ENOMEM or  
> ENOSPC?

No changed it (based on your suggestion) to EINVAL. ENOSPC is really
meant for disks out of space. So this actually kind of made the mode
since, it's an invalid argument, because it is too big.

> 
> > +   }
> >     xencomm_free(ports);
> > @@ -389,8 +394,8 @@ static int xenppc_privcmd_domctl(privcmd
> >     if (copy_to_user(user_op, &kern_op, sizeof(xen_domctl_t)))
> >             ret = -EFAULT;
> > -   xencomm_free(desc);
> > out:
> > +   xencomm_free(desc);
> >     xencomm_free(op_desc);
> >     return ret;
> > }
> > @@ -463,8 +468,8 @@ static int xenppc_privcmd_sysctl(privcmd
> >     if (copy_to_user(user_op, &kern_op, sizeof(xen_sysctl_t)))
> >             ret = -EFAULT;
> > -   xencomm_free(desc);
> > out:
> > +   xencomm_free(desc);
> >     xencomm_free(op_desc);
> >     return ret;
> > }
> > @@ -514,8 +519,8 @@ static int xenppc_privcmd_platform_op(pr
> >     if (copy_to_user(user_op, &kern_op, sizeof(xen_platform_op_t)))
> >             ret = -EFAULT;
> > -   xencomm_free(desc);
> > out:
> > +   xencomm_free(desc);
> >     xencomm_free(op_desc);
> >     return ret;
> > }
> > @@ -547,7 +552,10 @@ int HYPERVISOR_memory_op(unsigned int cm
> >                             sizeof(*xen_guest_handle(mop->extent_start)));
> >                     if (desc == NULL)
> > +                   {
> 
> Curlies again

ok I'll send a new patch with all the curlie braces on the first line.

> 
> > +                           xencomm_free(op_desc);
> >                             return -ENOMEM;
> > +                   }
> >                     set_xen_guest_handle(mop->extent_start,
> >                                          desc);
> 


_______________________________________________
Xen-ppc-devel mailing list
Xen-ppc-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-ppc-devel