Re: ztempfile

Raph,

> From: "Raph Levien" <[email protected]>
> To: "Igor V. Melichev" <[email protected]>
> Cc: <[email protected]>; "Ray Johnston" <[email protected]>;
"Raph Levien" <[email protected]>; "Raph Levien" <[email protected]>
> Sent: Friday, August 15, 2003 4:24 AM
> Subject: Re: ztempfile


> gp_gettmpdir() is perhaps poorly named - it merely retrieves a
> user-specified temp directory from the environment variables. From the
> name, one could wrongly guess that it actually determines the
> directory in all cases. In any case, I think my comments in the
> banner for gp_open_scratch_file() make it sufficiently clear that
> the responsibility for choosing the temp directory lies squarely with
> that function.

My vision is slightly different.
One of my general rules for programming is : if an object has no simple
name,
the object must not exist. In other words, objects must implement simple
concepts.
(A function is a kind of object).

The function gettmpdir appears a simple name,
which assumes a clear semantics - get a path to a directory for temporary
files.
It comes from C world and it is well known.
I assumed this semantics when coded ztempfile. You suggest to replace it
with something like
get_temp_directory_specification_from_environment.
It appears pretty clear as well, but more complicated than gettmpdir,
i.e. less preferrable.

Coding ztempfile, for gp_gettmpdir I assumed the semantics derived from
its name, and concluded that the environment variables are always set.
The second part of the assumption later appeared to be wrong -
sometimes it is not set, and the fuction fails. Note that the failure case
is not documented, so it was hard to guess.

Besides that, I consider the current implementation for gp_gettmpdir
to be a temporary solution, which was voluntary taken a long ago and then
forgotten.
The word "Currently" in the comment in gpmisc.h makes me to think so.

I guess we all agree that gp_gettmpdir to be improved, but disagree
about the direction of the improvement. My suggestion is to keep the old
name
and make the semantics to be consistent. Having a consistent gp_gettmpdir,
I can see a smooth solution for ztempfile :

- get a directory path;
- check it for permissions;
- open scratch file in there.

This logics does not set artifitial constraints like "simple" prefices, etc.
It is strongly straight-forward, and therefore it should be preferrable.

> We did talk about this case on the phone.

A Russian saying : "Good thoughts come afterwards".
I did not see a better solution during the discussion.
Now I see one. I'll make another patch shortly.

> > If this approach is taken, there is no need to change ztempfile.
> > I believe that the current code in CVS performs its job perfectly.
>
> I disagree strongly. One of the most important cases in which the
> current code fails is that it checks permission in the current
> directory, even for "simple" prefixes. This is wrong.

Probably I poorly explained. Assuming gp_gettmpdir always return
a non-empty path, and assuming that gp_open_scratch_file does not
prepend any own prefix, the current code for ztempfile appears perfect :

1. If the argument starts with root, the permissions are checked fine.
2. If the argument starts with cwd or parent, the permissions are checked
fine;
3. If the argument does not start with root, cwd or parent,
    it is prepended with a result of gp_gettmpdir,
    and then the permissions are checked fine.
4. The branch strcpy(tdir, gp_file_name_current());
    never executes;
5. There is an unobvious case, when gp_gettmpdir
    returns a path, which does not start with root, cwd or parent.
    The regular check for permissions is applied to it.
    It succeeds Iff the list permits it.
    Thus it is under user's control, and I conclude it is fine.

I don't see more cases. Do you ?


As to  the patch you suggested, I'll criticize it separately.

Igor.