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-ppc-devel mailing list Xenemail@example.com http://lists.xensource.com/xen-ppc-devel