Re: [Xen-devel] [PATCH v3 1/3] ring.h: introduce macros to handle monodirectional rings with multiple req sizes

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.03.17 at 22:48, <sstabellini@xxxxxxxxxx> wrote:
> On Thu, 23 Mar 2017, Stefano Stabellini wrote:
>> CC'ing Jan

As a first remark, I'm slightly confused by this being v3 when a
standalone v3 had been sent on Feb 22 already.

>> On Tue, 21 Mar 2017, Stefano Stabellini wrote:
>> > +static inline void name##_read_packet(const unsigned char *buf,           
>> >     \
>> > +        RING_IDX masked_prod, RING_IDX *masked_cons,                      
>> >     \
>> > +        RING_IDX ring_size, void *opaque, size_t size)                    
>> >     \
>> > +{                                                                         
>> >     \
>> > +    if (*masked_cons < masked_prod ||                                     
>> >     \
>> > +            size <= ring_size - *masked_cons) {                           
>> >     \
>> > +        memcpy(opaque, buf + *masked_cons, size);                         
>> >     \
>> > +    } else {                                                              
>> >     \
>> > +        memcpy(opaque, buf + *masked_cons, ring_size - *masked_cons);     
>> >     \
>> > +        memcpy((unsigned char *)opaque + ring_size - *masked_cons, buf,   
>> >     \
>> > +                size - (ring_size - *masked_cons));                       
>> >     \
>> > +    }                                                                     
>> >     \
>> > +    *masked_cons = name##_mask(*masked_cons + size, ring_size);           
>> >     \
>> > +}                                                                         
>> >     \
>> 
>> I like these macros, they make the code that uses them very nice, look
>> at patch #2 for example. So far, I tested them by importing them in
>> Linux and QEMU, I didn't realize that we have an -ansi check on the
>> public headers in Xen (see xen/include/Makefile:headers.chk).
>> 
>> Because of the static inline functions, there is no hope to compile them
>> with -ansi. As soon as we introduce the first user (9pfs, patch #2 of
>> this series), the compilation will break.
>> 
>> At the same time I am very keen on the static inlines and wouldn't want
>> to lose them.
>> 
>> 
>> Question 1: Should I move these useful macros elsewhere? If so, where?
>> Maybe I could move them to the spec, for example
>> docs/misc/9pfs.markdown. Xen doesn't really need them, it's just the
>> frontend and backend implementations that could benefit from them.
>> 
>> If we decide to keep them in ring.h, I guess I'll have to change the
>> headers.chk check in xen/include/Makefile for the 9pfs and pvcalls
>> headers to be -std=c99 (*only* for 9pfs and pvcalls, of course).
> 
> Actually, I noticed there is already a way to remove the ansi compliance
> check: I just need to add 9pfs and pvcalls to the filter-out list of
> PUBLIC_ANSI_HEADERS in xen/include/Makefile. Is that OK for you?

I think that's acceptable. They shouldn't go entirely unchecked
though, so I'd ask for adding a new C99 category, as you suggest
yourself above.

>> Question 2: In addition to the static inlines problem, the new macros
>> also use memcpy, that needs declaring. I could import <strings.h>, but I
>> don't think it makes sense in a Xen public header. Instead, would you
>> be OK with me adding the following to ring.h?
>> 
>> #include <stddef.h> /* needed for size_t */
>> extern void *memcpy(void *dest, const void *src, size_t s);
>> 
>> Of course, if we decide to move the new macros somewhere else, this
>> problem goes away with them.
> 
> I realized that stddef.h is not allowed either. I am not sure what to do
> here. If I remove the ansi check, actually these headers won't be
> involved in the build, so there won't be any breakages, and all users
> will have a memcpy defined. So maybe we could just get away without
> defining memcpy? Other suggestions?

We expect stdint.h to be included up front (or the types it
declares to be made available some other way); I don't see why
you shouldn't be allowed to expect the equivalent here before
including these new ones. Just make sure you prominently state
the prereqs (near the top of the headers, perhaps at the place
where one would normally expect such #include-s), in terms of
types and declarations (i.e. preferably not in terms of standard
header names, because of the permitted substitutions).

Jan

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