Re: [patch] filelocking, v2

[Floris Kraak <floris snow nl>]

On Mon, Jul 15, 2002 at 09:45:34AM -0600, Andreas J Guelzow wrote:
> Wouldn't
> lockfile = g_strconcat (filename, ".lck");
> be simpler ?

Probably. The problem is, I'm not really a C programmer - I
know the basics - functions, includes, all the basic programming
constructs - but when it comes to magic with strings and types...

Besides, it's not my code so I kindof have to trust the original
author that he got this part right.

> Why don't you use g_strdup_printf rather than this explicit calculation 
> and g_malloc?

Oh, I could use that.. as soon as I know how that works and what the
arguments are ;-) 

> "\0" rather than '\0' or 0?

Uhm .. does it make a difference? :-)

> > +                    gethostname(hostname2, 255);
>
>
> Why 255 and not 256 ? Moreover, are you sure that this is 0 terminated?

To your first question: I don't know. Looking at the manpage for
gethostname(), it sure looks like it could be '256'. Probably an
oversight, I pretty much left that unchanged. Doesn't hurt a lot,
just looks funny.

Secondly, according to the manpage, wether the result will be
null-terminated depends on wether it fits - if it doesn't, it gets
truncated and may or may not be null-terminated (unspecified).
Then .. hm well I suppose using strlen on it then would
be a real problem wouldn't it? Worst case, strlen returns an error
or some arbitrary size number and a malloc for some giant region could
get done .. uch. I'll see if I can solve that.

> - 
> gnum_file_saver_save (fs, io_context, wbv, file_name);
> +        if (!is_locked(file_name))
> +           gnum_file_saver_save (fs, io_context, wbv, file_name);
> +        else
> +           gnumeric_io_error_info_set(io_context, 
> error_info_new_str("File is in use!"));
>
>
> If I understand the code correctly, then this error message is 
> misleading: Your lockfile cannot be created which does not necessarily 
> mean that the file is in use by some other user! There are many other 
> potential reasons for not being able to save the lockfile in the default 
> directory.

Ah yes. Another symptom of patch crudeness. Keep in mind it was
developed over 6 months ago in an afternoon. The error message
can probably be improved. And as for is_locked() .. well I suppose I
do need to improve that test.

> Moreover "File is in use" is of course alwayd true, sicne it is 
> currently used by this process.
>
> In any case you are missing _(...)

I assume _( ) is some kind of string conversion macro? I'll add
it.

Anyway, thanks for the critical review, it needed it. I was told
this patch was offered before but not the reasons why it wasn't
included. Bas (Vermeulen, the primary author) told me he thought
it was probably just too crude and looking at your comments he's
probably right.

Jody said it might be a good idea to look at vim's locking code
and lift stuff from there. Sounds like a plan..

Regards,
Floris Kraak
-- 
Meddle not in the affairs of dragons, for thou art crunchy, and good
with ketchup.