Re: [PATCH] cp caching

[Jim Meyering]

Ondrej Oprala wrote:

> On 01/03/2013 02:12 PM, Jim Meyering wrote:
>>>         bool all_errors = !x->data_copy_required ||
>>> x->require_preserve_context;
>>>         bool some_errors = !all_errors && !x->reduce_diagnostics;
>> However, both of these declarations belong inside the if {...} body.
> What exactly do you mean? Moving the declarations deeper would put
> them out of the else {...} body's scope.

Those new variables are used only in the "if" body, so
their declarations belong there, too.

>> Also, please change the name and meaning of s/con_eq/context_change/
> Done.
...
> +++ b/src/copy.c
> @@ -2201,11 +2201,19 @@ copy_internal (char const *src_name, char const 
> *dst_name,
>      {
>        bool all_errors = !x->data_copy_required || 
> x->require_preserve_context;
>        bool some_errors = !all_errors && !x->reduce_diagnostics;
> +      bool context_change = 0;
>        security_context_t con;
> +      static security_context_t old_con;

We prefer not to use static variables like this in copy.c.  We want
this code to be library-safe, eventually, so at first I was going to say
that state could be accessible via the "struct cp_options *x" parameter.
But uh-oh, it's "const".  Add yet another parameter?  IMHO, there are
too many already.

Sorry I didn't notice that sooner.

Actually, I'm not sure we can safely perform this optimization, in general.
What is it doing?
It's recording that we've called setfscreatecon(con),
and if on a subsequent run through that same function
we call lgetfilecon and get a matching "con", then it
assumes we can skip the associated setfscreatecon call.

However, that assumption is valid only if the current *process*
has not called an intervening setfscreatecon(con2).  I.e., this change
widens the race condition window from a few instructions (between each
setfscreatecon call and subsequent file creation) to potentially
the entire duration of the copy process, since with your change,
there might be only one setfscreatecon call, just prior to the copying
of the first argument.  Then, any subsequent call to set a different
default FS context (before the opening of the final destination file)
would cause trouble.

Maybe we don't really care, because having to call setfscreatecon
(setting process-wide state) is so kludgey, the 25% improvement
is worth it, and besides, cp is certainly single-threaded.

The more I think about it, the more I like Pádraig's idea
of inheriting any improvement from setfscreatecon.