On Mon, Sep 24, 2012 at 10:05:02AM +0800, Miao Xie wrote: > Generally, we should check the value when it is input. If not, we might > run our program with the wrong value, and it is possible to cause unknown > problems. So I think the above chunk is necessary.
The difference is that giving an invalid value will exit early (your version), while Ilya's version will clamp the 'usage' to the allowed range during processing. From usability POV, I'd prefer to let the command fail early with a verbose error eg. that the range is invalid. > > (diff is on top of the patch in question) > > > > This is the most straightforward transformation I can think of. It > > doesn't result in an unnecessary BUG_ON, keeps churn to a minimum and > > agree with you. > > > doesn't change the "style" of the balance ioctl. (If I were to check > > every filter argument that way, btrfs_balance_ioctl() would be very long > > and complicated.) > > I think the check in btrfs_balance_ioctl() is necessary, the reason is above. btrfs_balance_ioctl does not seem as the right place, it does the processing related to the state of balance (resume/cancel etc). Look at btrfs_balance() itself, it does lot more sanity checks of the parameters. We can put the extra checks into helpers (and not only this one) if clarity and readability of the function becomes a concern. david -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
