On Fri, 2021-04-02 at 08:37 -0400, Laurence Oberman wrote:
> On Thu, 2021-04-01 at 18:33 -0700, Bart Van Assche wrote:
> > On 4/1/21 6:09 PM, Laurence Oberman wrote:
> > > #define sd_printk(prefix, sdsk, fmt, a...)
> > >
> > > \
> > > - (sdsk)->disk ?
> > > \
> > > - sdev_prefix_printk(prefix, (sdsk)->device, \
> > > + do {
> > > \
> > > + if (!storage_quiet_discovery)
> > > \
> > > + (sdsk)->disk ?
> > > \
> > > + sdev_prefix_printk(prefix, (sdsk)->device, \
> > > (sdsk)->disk->disk_name, fmt, ##a) :
> > > \
> > > - sdev_printk(prefix, (sdsk)->device, fmt, ##a)
> > > + sdev_printk(prefix, (sdsk)->device, fmt, ##a);
> > > \
> > > + } while (0)
> >
> > The indentation inside the above macro looks odd to me. I guess
> > that
> > you
> > want to avoid deep indentation? Consider using if (...) break;
> > instead
> > to reduce the indentation level. Additionally, please change the
> > ternary
> > operator into an if-condition since the result of the ternary
> > operator
> > is not used. How about rewriting the above macro as follows?
> >
> > do {
> > if (storage_quiet_discovery)
> > break;
> > if ((sdk)->disk)
> > [ ... ]
> > else
> > [ ... ]
> > } while (0)
> >
> > Thanks,
> >
> > Bart.
> >
>
> Hi Bart
> Yes, Thanks I can try that option for the macro and clean it up.
> I will wait a bit for others to review so I can attend to all changes
> at once.
>
> Note that the original version was indented like that too and has the
> ternary.
>
> See here:
> #define sd_printk(prefix, sdsk, fmt,
> a...) \
> (sdsk)->disk
> ? \
> sdev_prefix_printk(prefix, (sdsk)-
> > device, \
>
> (sdsk)->disk->disk_name, fmt, ##a)
> : \
> sdev_printk(prefix, (sdsk)->device, fmt, ##a)
>
Hi Bart the original macro is the same so I think best not to change it
other than the wrapper I added. What are your thoughts.
Gentle ping for any other reviews:
We are actively using this at some customers so it would be good to
know if others upstream will NAK this or have any comments.
Sincerely
Laurence Oberman
