[Xen-ia64-devel] Re: [XenPPC] RFC: xencomm - linux side

[Tristan Gingold <Tristan.Gingold@xxxxxxxx>]

Le Mercredi 23 AoÃt 2006 18:35, Hollis Blanchard a Ãcrit :
> On Wed, 2006-08-23 at 09:59 +0200, Tristan Gingold wrote:
> > Le Mardi 22 AoÃt 2006 21:03, Hollis Blanchard a Ãcrit :
> > > I don't understand the point of all these routines if they just call
> > > arch_foo anyways.
> >
> > Sorry I have not explained the principle.
> > xencomm_arch_hypercall_XXX are the raw hypercalls.  They must be defined
> > by architecture code. The xencomm_hypercall_XXX are the xencomm wrapper
> > and are shared.
>
> That much is clear. :) My question is what is being done in those "raw
> hypercalls" that can't be done here? You didn't include them in your
> patch, so I can't tell.
>
> It seems there are a few missing pieces to your patch. Next time please
> include the whole thing, including arch-specific parts, so we can see
> what's going on.
>
> > > > +       if (ret)
> > > > +               goto out; /* error mapping the nested pointer */
> > > > +
> > > > +       ret = xencomm_arch_hypercall_dom0_op (op_desc);
> > > > +
> > > > +       /* FIXME: should we restore the handle?  */
> > > > +       if (copy_to_user(user_op, &kern_op, sizeof(dom0_op_t)))
> > > > +               ret = -EFAULT;
> > > > +
> > > > +       if (desc)
> > > > +               xencomm_free(desc);
> > > > +out:
> > > > +       return ret;
> > > > +}
> > >
> > > You misplaced the out label; it needs to go before xencomm_free(desc);
> >
> > ??? This was copied from your work.
>
> You've made changes here, and that's what I'm pointing out.
>
> > The code branches to out iff xencomm allocation failed.  It is safe to
> > call xencomm_free but useless.
>
> There are multiple descriptors being created: one for the dom0_op
> top-level structure, and possibly one for a sub-structure. In fact, in
> your patch you never free 'op_desc' inside xencomm_privcmd_dom0_op().
> OK, reading closer, I don't like that at *all*. The trick is that
> xencomm_create_inline() doesn't actually create anything, and therefore
> you don't need to free it. That needs to change.
>
> My suggestion: have xencomm_create() test IS_KERNEL_ADDR() (in whatever
> way is best for portability) and if it is, do the "inline" stuff. On the
> free side, if the descriptor was inline, free can just return. That
> would also make me happy because it removes the need to think about
> whether callers can/should call "create_inline" or not; the code just
> does the right thing.
We definitly disagree here.  One whole point of xencomm_create_inline is it 
doesn't allocate memory and can't fail.  Because of that we don't need to 
worry about failure and freeing memory.  This makes the code a lot easier to 
write and to read.

Tristan.

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