Re: [RFC] Support for bzip compressed modules

[James Antill <jantill@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3172 bytes --]

On Mon, 2007-01-08 at 15:34 -0500, Karl MacMillan wrote:

> The patch implements this support by changing sepol_policy_file_t to 
> support decompressing files or memory areas into a private memory copy. 
> This support is optional - dlopen is used so that a hard dependency to 
> libbz2 is not introduced. I took the approach of decompressing the 
> entire file or memory area because:

 Why don't we want to depend on libbz, if we are building with bz2
support? 

> * It is very simple
> * The current code depends on the ability to seek within policy files - 
> this is not really possible within compressed streams using the bzip2 
> library.
> 
> The downsides are:
> 
> * Increased memory usage
> * No transparent support for compressed writing with an fd based policy 
> file.
> 
> I didn't want to add additional set functions - I would have preferred 
> to allow sepol_policy_file_set_[mem,fd] to transparently open compressed 
> streams with functions to set other behaviors as options stored in 
> sepol_policy_file_t structs. This was not possible becuase the current 
> set functions do not return errors.

 Do we really care about the memory usage, my instinct would be to drop
the FILE specific code and just dump everything into memory and then
call the mem_set function and thus. have only one decompression loop
(adding the fd version is simple then too).
 Calling fstat(fileno(fp)) to read the policy in is probably easier than
a loop.

> Comments appreciated. Some very crude benchmarking below (note that I am 
> using a patched semodule to allow the globbing syntax - patch for that 
> to follow). The summary is that there is substantial space savings at 
> the expense of some increase in time to complete common actions. An 
> acceptable trade-off in my opinion.
> 
> Anyone have suggestions for something as simple as time but for max 
> memory usage?

 There's memusage in glibc-utils.

---- code ----

 The bz2 code looks fine, although the += BUFSIZE in one loop and *= 2
in the other is weird, and there's a couple of minor nits in the
interface:

. check is always true in callers, and I'm not sure why you'd have it
zero.

. All code paths have:

 if (set_foo_bz2() == FAILED)
  set_foo();

...which tells me set_foo_bz2() should do that ... in fact it seems sane
to just change set_foo() to check of bz2ness and do the right thing,
without having to alter the callers.

. A personal minor nit is that free(NULL) works fine, so don't work
around it (this idiom seems to be used in sepol).

. sepol_policy_file_free_data() is also called multiple times at the end
of the set_foo_bz2() functions (once inside set_foo() and then
explicitly immediately after).


 I assume the only reason you went with bzip2 over gzip is the "have to
init yourself in the set_mem case"? I've done that before[1], so I can
help you get that bit done if you want ... this will drop
CPU/memory/dependency requirements (although expecting all Linux to have
libbz now isn't a big deal, IMO).


[1] http://www.and.org/vstr/examples/ex_zcat.c


-- 
James Antill <jantill@redhat.com>

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]