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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.08.12 at 16:45, Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
wrote:
> On Fri, 17 Aug 2012, 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 is how it would look like:
> 
> #define guest_handle_to_param(hnd, type) ({                  \
>     /* type checking: make sure that the pointers inside     \
>      * XEN_GUEST_HANDLE and XEN_GUEST_HANDLE_PARAM are of    \
>      * the same type, than return hnd */                     \
>     (void)((typeof(&(hnd).p)) 0 ==                           \
>         (typeof(&((XEN_GUEST_HANDLE_PARAM(type)) {}).p)) 0); \
>     (hnd);                                                   \
> })
> 
> 
> Honestly I have very rarely seen anything less readable, but at least is
> very compact.
> For ARM I was going to go with the following, that is only slightly more
> readable:
> 
> #define guest_handle_to_param(hnd, type) ({                  \
>     typeof((hnd).p) _x = (hnd).p;                            \
>     XEN_GUEST_HANDLE_PARAM(type) _y = { _x };                \
>     /* type checking: make sure that the pointers inside     \
>      * XEN_GUEST_HANDLE and XEN_GUEST_HANDLE_PARAM are of    \
>      * the same type, than return hnd */                     \
>     (void)(&_x == &_y.p);                                    \
>     _y;                                                      \
> })

Yes, this looks good now to me (perhaps correct the minor
spelling mistake - should be "then", not "than" I think).

Jan

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