Re: [Xen-devel] [PATCH v5 09/15] argo: implement the sendv op; evtchn: expose send_guest_global_virq

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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:
> > >  do_argo_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg1,
> > >             XEN_GUEST_HANDLE_PARAM(void) arg2, unsigned long arg3,
> > > @@ -1145,6 +1734,53 @@ do_argo_op(unsigned int cmd, 
> > > XEN_GUEST_HANDLE_PARAM(void) arg1,
> > >          break;
> > >      }
> > >
> > > +    case XEN_ARGO_OP_sendv:
> > > +    {
> > > +        xen_argo_send_addr_t send_addr;
> > > +
> > > +        XEN_GUEST_HANDLE_PARAM(xen_argo_send_addr_t) send_addr_hnd =
> > > +            guest_handle_cast(arg1, xen_argo_send_addr_t);
> > > +        XEN_GUEST_HANDLE_PARAM(xen_argo_iov_t) iovs_hnd =
> > > +            guest_handle_cast(arg2, xen_argo_iov_t);
> > > +        /* arg3 is niov */
> > > +        /* arg4 is message_type. Must be a 32-bit value. */
> > > +
> > > +        rc = copy_from_guest(&send_addr, send_addr_hnd, 1) ? -EFAULT : 0;
> > > +        if ( rc )
> > > +            break;
> > > +
> > > +        /*
> > > +         * 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)".

IMO it's much more likely to miss an 'f' in the first construct, and
thus get the value wrong and introduce a bug.

Anyway, this is your code, so I'm not going to insist.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel