Re: [Xen-devel] [PATCH 1 of 7 v2] blktap3/tapback: Introduce core defines and structure definitions

[Thanos Makatos <thanos.makatos@xxxxxxxxxx>]

> On Fri, 2013-01-04 at 12:14 +0000, Thanos Makatos wrote:
> > +#define BUG_ON(_cond)       \
> > +    if (unlikely(_cond)) {  \
> > +        assert(0);          \
> 
> This is basically a fancy way to write "assert(!_cond)" I think? I
> don't
> imagine that the unlikely will make a measurable difference for you but
> the downside is that a clever assert will print out the condition, so
> you get
>       assertion failed: 0
> instead of
>       assertion failed: <_cond>
> 
> So you might almost as well use either abort() or
>       if (unlikely(_cond))
>               assert(!_cond)
> 
> (evaluating cond twice won't hurt much under the circumstances, and the
> compiler will probably optimise that anyway)

This #define shouldn't be there in the first place (that's the kernel's way), 
assert(!_cond) should suffice.

> 
> > +    }
> > +
> > +/*
> > + * XenStore path components.
> > + *
> > + * To avoid confusion with blktap2, we'll use a new kind of device
> for libxl
> > + * defining it in tools/libxl/libxl_types_internal.idl. This will be
> done by
> > + * the patch that adds libxl support for blktap3. TODO When that
> patch is sent,
> > + * use the definition from there instead of hard-coding it here.
> > + */
> > +#define XENSTORE_BACKEND        "backend"
> > +#define BLKTAP3_BACKEND_NAME    "xenio"
> 
> tapback?

Xenio is the back-end's name (e.g "vbd" in blktap2), so it's shouldn't be 
strongly related to the daemon's name. In any case, at some point (hopefully 
;)) blktap3 will become the default back-end and should use the "vbd" back-end 
name, so I don't think we should bother finding a pretty, temporary name for 
this.

> 
> > +#define BLKTAP3_BACKEND_PATH
> XENSTORE_BACKEND"/"BLKTAP3_BACKEND_NAME
> > +#define BLKTAP3_BACKEND_TOKEN   XENSTORE_BACKEND"-
> "BLKTAP3_BACKEND_NAME
> > +#define BLKTAP3_FRONTEND_TOKEN  "otherend-state"
> > +#define BLKTAP3_SERIAL          BLKTAP3_BACKEND_NAME"-serial"
> 
> I'm not sure all of these are worth a #define, but I'm not yet sure
> what
> they are used for.

They're used for querying and parsing XenStore paths. IMO it's better, from a 
defensive programming perspective, to avoid using the same string multiple 
times as it makes it harder to maintain the code.

> > +#define FEAT_PERSIST            "feature-persistent"
> > +/**
> > + * Iterates over all devices and returns the one for which the
> condition is
> > + * true.
> > + */
> > +#define tapback_backend_find_device(_device, _cond)                \
> > +do {
>                       \
> > +   vbd_t *__next;
> \
> > +   int found = 0;
>               \
> > +   tapback_backend_for_each_device(_device, __next) {      \
> > +           if (_cond) {
>               \
> > +                   found = 1;
>               \
> > +                   break;
>                       \
> > +           }
>                       \
> > +   }
>                       \
> > +   if (!found)
>               \
> > +           _device = NULL;
>               \
> > +} while (0)
> 
> Whitespace...
> 
Fixed.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel