On Sun, Sep 15, 2019 at 04:30:04PM +0200, Jinpu Wang wrote:
> Thanks Bart for detailed review, reply inline.
>
> On Sat, Sep 14, 2019 at 12:10 AM Bart Van Assche <bvanassche@xxxxxxx> wrote:
> >
> > On 6/20/19 8:03 AM, Jack Wang wrote:
> > > +#define ibnbd_log(fn, dev, fmt, ...) ({ \
> > > + __builtin_choose_expr( \
> > > + __builtin_types_compatible_p( \
> > > + typeof(dev), struct ibnbd_clt_dev *), \
> > > + fn("<%s@%s> " fmt, (dev)->pathname, \
> > > + (dev)->sess->sessname, \
> > > + ##__VA_ARGS__), \
> > > + __builtin_choose_expr( \
> > > + __builtin_types_compatible_p(typeof(dev), \
> > > + struct ibnbd_srv_sess_dev *), \
> > > + fn("<%s@%s>: " fmt, (dev)->pathname, \
> > > + (dev)->sess->sessname, ##__VA_ARGS__), \
> > > + unknown_type())); \
> > > +})
> >
> > Please remove the __builtin_choose_expr() /
> > __builtin_types_compatible_p() construct and split this macro into two
> > macros or inline functions: one for struct ibnbd_clt_dev and another one
> > for struct ibnbd_srv_sess_dev.
> Ok, will split to two macros.
>
> >
> > > +#define IBNBD_PROTO_VER_MAJOR 2
> > > +#define IBNBD_PROTO_VER_MINOR 0
> > > +
> > > +#define IBNBD_PROTO_VER_STRING __stringify(IBNBD_PROTO_VER_MAJOR) "." \
> > > + __stringify(IBNBD_PROTO_VER_MINOR)
> > > +
> > > +#ifndef IBNBD_VER_STRING
> > > +#define IBNBD_VER_STRING __stringify(IBNBD_PROTO_VER_MAJOR) "." \
> > > + __stringify(IBNBD_PROTO_VER_MINOR)
> >
> > Upstream code should not have a version number.
> IBNBD_VER_STRING can be removed together with MODULE_VERSION.
> >
> > > +/* TODO: should be configurable */
> > > +#define IBTRS_PORT 1234
> >
> > How about converting this macro into a kernel module parameter?
> Sounds good, will do.
Don't rush to do it and defer it to be the last change before merging,
this is controversial request which not everyone will like here.
Thanks
