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 <[email protected]> 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
