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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 24 Mar 2017, Jan Beulich wrote:
> >>> 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.

Yes, sorry, I actually sent 4 versions of this patch as standalone.
However, after adding the other two patches I reset the version count.


> >> 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.

OK, that makes sense.


> >> 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).

OK, that is a good suggestion, I'll do that.

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