On Wed, Sep 14, 2016 at 03:37:01PM -0600, Andreas Dilger wrote: > On Sep 14, 2016, at 2:57 PM, Eric Biggers <ebiggers@xxxxxxxxxx> wrote: > > > > This makes the return value match the comment. Previously it would > > actually return 0 if encryption was successful. No callers currently > > care, but this change should reduce the chance of future bugs. > > This may be introducing a subtle bug in fscrypt_fname_usr_to_disk(), since > that function returns the status from fname_encrypt() directly and now it > returns the name length instead of 0 on success: > fscrypt_fname_usr_to_disk() already returned a length in the "." and ".." cases. So any caller which assumed it returned 0 on success would have already been buggy. Fortunately, there aren't any such callers currently. > > This percolates further up to some of the callers, but in the cases that I > saw the check is "if (err < 0)" and the positive value is either ignored > or overwritten before being returned further up the call chain. However, > that could be easily missed in the future and somewhere up the call chain > doing "if (rc)" would suddenly start to fail. > > Since both "struct fscrypt_str" and "struct qstr" already hold the length > I don't think there is any benefit to returning the length to the caller. > Since (IMHO) this creates a non-trivial chance of introducing bugs in the > future it makes more sense to just change the function comment to match the > actual behaviour. > I agree that the return value is redundant and somewhat error prone. However, this style is already being used for fscrypt_fname_disk_to_usr(), fscrypt_fname_usr_to_disk(), and fname_decrypt(). My patch was primarily intended to make things more consistent by updating fname_encrypt(), which was the odd one out. If you'd prefer, I can instead do a patch to make all these related functions return 0 on success, rather than a length. That would be a somewhat larger patch. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
