[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v5 09/15] argo: implement the sendv op; evtchn: expose send_guest_global_virq
On Sun, Feb 03, 2019 at 09:56:26AM -0800, Christopher Clark wrote: > On Thu, Jan 31, 2019 at 3:01 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote: > > > > On Thu, Jan 31, 2019 at 03:35:23AM -0700, Jan Beulich wrote: > > > >>> On 31.01.19 at 11:18, <roger.pau@xxxxxxxxxx> wrote: > > > > On Wed, Jan 30, 2019 at 08:10:28PM -0800, Christopher Clark wrote: > > > >> On Tue, Jan 22, 2019 at 4:08 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> > > > >> wrote: > > > >> > On Mon, Jan 21, 2019 at 01:59:49AM -0800, Christopher Clark wrote: > > > >> > > + /* > > > >> > > + * Check padding is zeroed. Reject niov above limit or > > > >> > > message_types > > > >> > > + * that are outside 32 bit range. > > > >> > > + */ > > > >> > > + if ( unlikely(send_addr.src.pad || send_addr.dst.pad || > > > >> > > + (arg3 > XEN_ARGO_MAXIOV) || (arg4 & > > > >> > > ~0xffffffffUL)) ) > > > >> > > > > >> > arg4 & (GB(4) - 1) > > > >> > > > > >> > Is clearer IMO, or: > > > >> > > > > >> > arg4 > UINT32_MAX > > > >> > > > >> I've left the code unchanged, as the mask constant is used multiple > > > >> places elsewhere in Xen. UINT32_MAX is only used as a threshold value. > > > > > > > > The fact that others parts of the code could be improved is not an > > > > excuse to follow suit. I'm having a hard time believing that you find > > > > "arg4 & ~0xffffffffUL" easier to read than "arg4 & ~(GB(4) - 1)" or > > > > even "arg4 >= GB(4)". > > > Below, I propose an alternative way of achieving our correctness and > readability goals. > > On the topic of readability, this self-contained definition > does stand out: ~0xffffffffUL, > encouraging caution and careful counting of 'f's. However, no other > source files are involved, making the code independent of changes in > (macro) definitions in other files. > > In comparison, to understand GB, I have find the external definition, > and then parse this: > > #define GB(_gb) (_AC(_gb, ULL) << 30) > > (which seems to have a different type? ULL vs UL?) and then find and > understand this, in another file: > > #ifdef __ASSEMBLY__ > #define _AC(X,Y) X > #define _AT(T,X) X > #else > #define __AC(X,Y) (X##Y) > #define _AC(X,Y) __AC(X,Y) > #define _AT(T,X) ((T)(X)) > #endif > > so I'm saying: it's at least somewhat arguable which is easier to understand. > Regardless, I think there's a better option than either. > > > > > IMO it's much more likely to miss an 'f' in the first construct, and > > > > thus get the value wrong and introduce a bug. > > > > > > I agree with this last statement, but I'm having trouble to see how > > > message _type_ is related to a size construct like GB(4) is. I see > > > only UINT32_MAX as a viable alternative for something that's not > > > expressing the size of anything. > > > > I've suggested the GB construct as an alternative because the comment > > above mentions the 32bit range. IMO anything that avoids using > > 0xffffffffUL is fine. > > Jan and Andrew have employed a useful technique in recent changes where such a > test was required. This could work: > > (arg4 != (uint32_t)arg4)) > > It is self-contained, readable and clearly expresses the intent of the check > being performed. I have tested a series with this applied, and have it ready > to post if you approve. Yes, that's fine. As said in v7 anything that doesn't involve an open-coded mask is fine with me. Roger. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |