Re: [PATCH] ima-evm-utils: Namespace some too generic function names

[Mimi Zohar <zohar@linux.ibm.com>]

On Thu, 2019-07-25 at 04:53 +0300, Vitaly Chikunov wrote:
> Mimi,
> 
> On Wed, Jul 24, 2019 at 07:24:20PM -0400, Mimi Zohar wrote:
> > On Wed, 2019-07-24 at 23:42 +0300, Vitaly Chikunov wrote:
> > > Prefix `dump', `do_dump', and `params' with `ima_' to avoid colliding
> > > with other global symbols.
> > 
> > The package is named ima-evm-utils, the tool is named evmctl, and now
> > we're prefixing the global symbols with "ima".  Some of the functions,
> > like dump(), are used by both "ima" and "evm".  Aiming for some sort
> > of consistency, maybe it should be prefixed with "ima_evm", not just
> > "ima_"? 
> 
> Just ima_ is OK with me. EVM could be thought as IMA extension.

At least in the kernel, I've tried really hard to keep them as two
independent subsystems.  Does it make sense to use EVM without IMA,
probably not.  The EVM design allows for other subsystems, not only
IMA, to verify the file metdata integrity.  In fact, I heard about
some plans, relatively recently, to do so.

> Or we can use evm_ like in evmctl. Or imaevm_ (without underscore, like
> in libimaevm or imaevm.h).

There's already a lot of confusion as to what is "IMA".  Not only can
IMA be configured to store measurements, but can also be configured to
verify file signatures/hashes and audit file hashes.  Not that anyone
is looking at the naming details in this code, but I don't think we
should add to the confusion.  Could we use "imaevm_" as you suggested?

struct libimaevm_params {
        int verbose;
        int x509;
        const char *hash_algo;
        const char *keyfile;
        const char *keypass;
};

extern struct libimaevm_params ima_params;

imaevm_params?

> 
> > dump() should never have been named just "dump".  It should have at
> > least been named "hexdump".
> >  
> > > `params' is prefixed with a #define trick to avoid change in half
> > > hundred places.
> > 
> > Perhaps separate this change from the other change?
> 
> I agree to Bruno E. O. Meneguele it's better to actually rename `params'
> like other functions instead of redefining. Then all renames can go in
> one commit?

Sure.

"get_hash_algo()" can't be made static as it is being called from
hash_ima().  Could you also include renaming "get_hash_algo()" as
well?

Thanks!

Mimi