| Message ID | 20200228212814.105897-1-Jes.Sorensen@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Split fsverity-utils into a shared library | expand |
On 2/28/20 4:28 PM, Jes Sorensen wrote: > From: Jes Sorensen <jsorensen@fb.com> > > Hi, > > Here is a reworked version of the patches to split fsverity-utils into > a shared library, based on the feedback for the original version. Note > this doesn't yet address setting the soname, and doesn't have the > client (rpm) changes yet, so there is more work to do. > > Comments appreciated. Hi, Any thoughts on this patchset? Thanks, Jes > Cheers, > Jes > > Jes Sorensen (6): > Build basic shared library framework > Change compute_file_measurement() to take a file descriptor as > argument > Move fsverity_descriptor definition to libfsverity.h > Move hash algorithm code to shared library > Create libfsverity_compute_digest() and adapt cmd_sign to use it > Introduce libfsverity_sign_digest() > > Makefile | 18 +- > cmd_enable.c | 11 +- > cmd_measure.c | 4 +- > cmd_sign.c | 526 ++++---------------------------------------------- > fsverity.c | 15 ++ > hash_algs.c | 26 +-- > hash_algs.h | 27 --- > libfsverity.h | 99 ++++++++++ > libverity.c | 526 ++++++++++++++++++++++++++++++++++++++++++++++++++ > util.h | 2 + > 10 files changed, 707 insertions(+), 547 deletions(-) > create mode 100644 libfsverity.h > create mode 100644 libverity.c >
On Tue, Mar 10, 2020 at 04:32:12PM -0400, Jes Sorensen wrote: > On 2/28/20 4:28 PM, Jes Sorensen wrote: > > From: Jes Sorensen <jsorensen@fb.com> > > > > Hi, > > > > Here is a reworked version of the patches to split fsverity-utils into > > a shared library, based on the feedback for the original version. Note > > this doesn't yet address setting the soname, and doesn't have the > > client (rpm) changes yet, so there is more work to do. > > > > Comments appreciated. > > Hi, > > Any thoughts on this patchset? > > Thanks, > Jes > 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. 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 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? Can you make sure that the set of error codes for each API function is clearly defined? Can you make sure all API functions return an error if any reserved fields are set? Do you have a pointer to the corresponding RPM patches that will use this? Also, it would be nice if you could also add some tests of the API to fsverity-utils itself :-) - Eric
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
From: Jes Sorensen <jsorensen@fb.com> Hi, Here is a reworked version of the patches to split fsverity-utils into a shared library, based on the feedback for the original version. Note this doesn't yet address setting the soname, and doesn't have the client (rpm) changes yet, so there is more work to do. Comments appreciated. Cheers, Jes Jes Sorensen (6): Build basic shared library framework Change compute_file_measurement() to take a file descriptor as argument Move fsverity_descriptor definition to libfsverity.h Move hash algorithm code to shared library Create libfsverity_compute_digest() and adapt cmd_sign to use it Introduce libfsverity_sign_digest() Makefile | 18 +- cmd_enable.c | 11 +- cmd_measure.c | 4 +- cmd_sign.c | 526 ++++---------------------------------------------- fsverity.c | 15 ++ hash_algs.c | 26 +-- hash_algs.h | 27 --- libfsverity.h | 99 ++++++++++ libverity.c | 526 ++++++++++++++++++++++++++++++++++++++++++++++++++ util.h | 2 + 10 files changed, 707 insertions(+), 547 deletions(-) create mode 100644 libfsverity.h create mode 100644 libverity.c