On Thu, Sep 19, 2019 at 10:52:41PM +0000, Michael Kelley wrote:
> From: Branden Bonaby <brandonbonaby94@xxxxxxxxx> Sent: Thursday, September 12, 2019 7:32 PM
> >
> > +
> > +static int hv_debugfs_delay_set(void *data, u64 val)
> > +{
> > + int ret = 0;
> > +
> > + if (val >= 0 && val <= 1000)
> > + *(u32 *)data = val;
> > + else
> > + ret = -EINVAL;
> > +
> > + return ret;
> > +}
>
> I should probably quit picking at your code, but I'm going to
> do it one more time. :-)
>
> The above test for val >=0 is redundant as 'val' is declared
> as 'u64'. As an unsigned value, it will always be >= 0. More
> broadly, the above function could be written as follows
> with no loss of clarity. This accomplishes the same thing in
> only 4 lines of code instead of 6, and the main execution path
> is in the sequential execution flow, not in an 'if' statement.
>
> {
> if (val > 1000)
> return -EINVAL;
> *(u32 *)data = val;
> return 0;
> }
>
> Your code is correct as written, so this is arguably more a
> matter of style, but Linux generally likes to do things clearly
> and compactly with no extra motion.
>
Yea the less than 0 comparison isnt needed, so I'll update that
> +/* Delay buffer/message reads on a vmbus channel */
> > +void hv_debug_delay_test(struct vmbus_channel *channel, enum delay delay_type)
> > +{
> > + struct vmbus_channel *test_channel = channel->primary_channel ?
> > + channel->primary_channel :
> > + channel;
> > + bool state = test_channel->fuzz_testing_state;
> > +
> > + if (state) {
> > + if (delay_type == 0)
> > + udelay(test_channel->fuzz_testing_interrupt_delay);
> > + else
> > + udelay(test_channel->fuzz_testing_message_delay);
>
> This 'if/else' statement got me thinking. You have an enum declared below
> that lists the two options -- INTERRUPT_DELAY or MESSAGE_DELAY. The
> implication is that we might add more options in the future. But the
> above 'if/else' statement isn't really set up to easily add more options, and
> the individual fields for fuzz_testing_interrupt_delay and
> fuzz_testing_message_delay mean adding more branches to the 'if/else'
> statement whenever a new DELAY type is added to the enum. And the
> same is true when adding the entries into debugfs. A more general
> solution might use arrays and loops, and treat the enum value as an
> index into an array of delay values. Extending to add another delay type
> could be as easy as adding another entry to the enum declaration.
>
> The current code is for the case where n=2 (i.e., two different delay
> types), and as such probably doesn't warrant the full index/looping
> treatment. But in the future, if we add additional delay types, we'll
> probably revise the code to do the index/looping approach.
>
> So to be clear, at this point I'm not asking you to change the existing
> code. My comments are more of an observation and something to
> think about in the future.
>
I do see your point, thanks for the input. I think since its just two
it might be better to leave it but it definitely makes sense.
> >
> > +enum delay {
> > + INTERRUPT_DELAY = 0,
> > + MESSAGE_DELAY = 1,
> > +};
> > +
>
> Michael
