On 3/10/20 5:10 PM, Eric Biggers wrote: > On Tue, Mar 10, 2020 at 04:32:12PM -0400, Jes Sorensen wrote: >> On 2/28/20 4:28 PM, Jes Sorensen wrote: >> Any thoughts on this patchset? >> >> Thanks, >> Jes Hi Eric, Thanks for the quick response, a couple of comments: > It's been on my list of things to review but I've been pretty busy. But a few > quick comments now: > > The API needs documentation. It doesn't have to be too formal; comments in > libfsverity.h would be fine. Absolutely, I wanted to at least roughly agree on the interfaces before starting to do that. > Did you check that the fs-verity xfstests still pass? They use fsverity-utils. > See: https://www.kernel.org/doc/html/latest/filesystems/fsverity.html#tests I didn't, I will make sure to test this. > struct fsverity_descriptor and struct fsverity_hash_alg are still part of the > API. But there doesn't seem to be any point in it. Why aren't they internal to > libfsverity? I agree fsverity_descriptor should stay internal. I think struct fsverity_hash_alg is better exposed. It provides useful information for the caller, in particular the digest_size and allows for walking the list of supported algorithms, which again makes it possible to implement show_all_hash_algs(). If we kill it I would need to provide a libfsverity_get_digest_size() call as a minimum. > Can you make sure that the set of error codes for each API function is clearly > defined? I will go over this! > Can you make sure all API functions return an error if any reserved fields are > set? Good point, I'll look into this. > Do you have a pointer to the corresponding RPM patches that will use this? That's my next job, will post something as soon as I have something useful. > Also, it would be nice if you could also add some tests of the API to > fsverity-utils itself :-) Point taken :-) Thanks, Jes
|