Sorry to reply late. On Mon, 24 Sep 2012 18:47:42 +0200, David Sterba wrote: >>> 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
I think we should not put the check in btrfs_balance(), because the arguments are valid forever if they pass the check when they are input, if we put the check in btrfs_balance(), the check will be done every time we resume the balance. it is unnecessary. > We can put the extra checks into helpers (and not only this > one) if clarity and readability of the function becomes a concern. Agree. I will put this check into a helper in the next version of this patch. And I will make a separate patch to move the current check in btrfs_balance from btrfs_balance to the above helper after this patch is received. Thanks Miao -- 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
