mbox series

[v2,0/6] Split fsverity-utils into a shared library

Message ID 20200228212814.105897-1-Jes.Sorensen@gmail.com
Headers show
Series Split fsverity-utils into a shared library | expand

Message

Jes Sorensen Feb. 28, 2020, 9:28 p.m. UTC
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

Comments

Jes Sorensen March 10, 2020, 8:32 p.m. UTC | #1
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
>
Eric Biggers March 10, 2020, 9:10 p.m. UTC | #2
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
Jes Sorensen March 10, 2020, 9:53 p.m. UTC | #3
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