Re: [Xen-devel] [PATCH v3 4/6] xen: introduce XEN_GUEST_HANDLE_PARAM

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2012-08-17 at 15:57 +0100, Jan Beulich wrote:
> >>> On 17.08.12 at 16:50, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> > On Fri, 2012-08-17 at 14:58 +0100, Jan Beulich wrote:
> >> >>> On 17.08.12 at 15:47, Stefano Stabellini 
> >> >>> <stefano.stabellini@xxxxxxxxxxxxx>
> >> wrote:
> >> > On Fri, 17 Aug 2012, Jan Beulich wrote:
> >> >> >>> On 17.08.12 at 10:02, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> >> >> > On Thu, 2012-08-16 at 18:10 +0100, Stefano Stabellini wrote:
> >> >> >> On Thu, 16 Aug 2012, Jan Beulich wrote:
> >> >> >> > >>> On 16.08.12 at 17:54, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> >> >> >> > > Seeing the patch I btw realized that there's no easy way to
> >> >> >> > > avoid having the type as a second argument in the conversion
> >> >> >> > > macros. Nevertheless I still don't like the explicitly specified 
> >> >> >> > > type
> >> >> >> > > there.
> >> >> >> > 
> >> >> >> > Btw - on the architecture(s) where the two handles are identical
> >> >> >> > I would prefer you to make the conversion functions trivial (and
> >> >> >> > thus avoid making use of the "type" parameter), thus allowing
> >> >> >> > the type checking to occur that you currently circumvent.
> >> >> >> 
> >> >> >> OK, I can do that.
> >> >> > 
> >> >> > Will this result in the type parameter potentially becoming stale?
> >> >> > 
> >> >> > Adding a redundant pointer compare is a good way to get the compiler 
> >> >> > to
> >> >> > catch this. Smth like;
> >> >> > 
> >> >> >         /* Cast a XEN_GUEST_HANDLE_PARAM to XEN_GUEST_HANDLE */
> >> >> >         #define guest_handle_from_param(hnd, type) ({
> >> >> >             typeof((hnd).p) _x = (hnd).p;
> >> >> >             XEN_GUEST_HANDLE(type) _y;
> >> >> >             &_y == &_x;
> >> >> >             hnd;
> >> >> >          })
> >> >> 
> >> >> Ah yes, that's a good suggestion.
> >> >> 
> >> >> > I'm not sure which two pointers of members of the various structs need
> >> >> > to be compared, maybe it's actually &_y.p and &hnd.p, but you get the
> >> >> > idea...
> >> >> 
> >> >> Right, comparing (hnd).p with _y.p would be the right thing; no
> >> >> need for _x, but some other (mechanical) adjustments would be
> >> >> necessary.
> >> > 
> >> > The _x variable is still useful to avoid multiple evaluations of hnd,
> >> > even though I know that this is not a public header.
> >> 
> >> But we had settled on returning hnd unmodified when both
> >> handle types are the same.
> >> 
> >> > What about the following:
> >> > 
> >> > /* Cast a XEN_GUEST_HANDLE to XEN_GUEST_HANDLE_PARAM */
> >> > #define guest_handle_to_param(hnd, type) ({                \
> >> >     typeof((hnd).p) _x = (hnd).p;                          \
> >> >     XEN_GUEST_HANDLE_PARAM(type) _y = { _x };              \
> >> >     if (&_x != &_y.p) BUG();                               \
> >> >     _y;                                                    \
> >> > })
> >> 
> >> Since this is not a public header, something like this (untested,
> >> so may not compile as is)
> >> 
> >> #define guest_handle_to_param(hnd, type) ({                \
> >>     (void)(typeof((hnd).p)0 == (XEN_GUEST_HANDLE_PARAM(type){}).p); \
> >>     (hnd);                                                    \
> >> })
> >> 
> >> is what I was thinking of.
> > 
> > This evaluates hnd twice, or do we only care about that in public
> > headers for some reason? (personally I think principal of least surprise
> > suggests avoiding it wherever possible)
> 
> No, it doesn't - like sizeof(), typeof() doesn't evaluate its
> argument.

Right, of course, silly me.

Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel