Re: [patch] filelocking, v2

[Andreas J Guelzow <aguelzow taliesin ca>]

Floris Kraak wrote:

> Hi,
>
> The attached patch implements filelocking in gnumeric.
> I've cleaned up the way the lockstring is allocated a bit,
> please consider including this.

> +
> +       lockfile = g_malloc(strlen(filename) + 5);
> +       strcpy(lockfile, filename);
> +       strcat(lockfile, ".lck");


Wouldn't
lockfile = g_strconcat (filename, ".lck");
be simpler ?


> +
> +       if ((fd = open(lockfile, O_WRONLY|O_CREAT|O_EXCL, S_IREAD|S_IWRITE)) >= 0) {
> +               /* Lock succeeded */
> +               char *lockstr;
> +               char hostname[256];
> +               int stringsize; // size of locking string
> +               long pid;
> +		    
> +		/* Create the locking string, format is 
> +		 * hostname:pid 
> +		 * FBK 15-9-2002: cleanup memory allocation and pid use
> +                */
> +               gethostname(hostname, 255);
> +               pid = (long) getpid();
> +		stringsize = strlen(hostname) + ( 4 * sizeof (long) ) +1; 
> +		lockstr = g_malloc(stringsize);
> +               
> +               snprintf(lockstr, stringsize, "%s:%ld", hostname, pid);


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



> +               printf("DEBUG: Locking %s with %s\n", lockfile, lockstr);
> +               write(fd, lockstr, strlen(lockstr));
> +               close(fd);
> +               g_free(lockstr);
> +
> +               retval = TRUE;
> +	} 
> +	g_free(lockfile);
> +
> +       return retval;
> +}
> +
> +gboolean relinquish_lock(const char* filename)
> +{
> +       char *lockfile;
> +       gboolean retval = FALSE;
> +       int fd;
> +
> +       g_return_val_if_fail(filename != NULL, FALSE);
> +       g_return_val_if_fail(strlen(filename) != 0, FALSE);
> +
> +       lockfile = g_malloc(strlen(filename) + 5);
> +       strcpy(lockfile, filename);
> +       strcat(lockfile, ".lck");
> +
> +       if ((fd = open(lockfile, O_RDONLY)) >= 0) {
> +               char lockstr[300] = "\0";
> +               int pid = 0;
> +               if (read(fd, lockstr, 300) >= 0) {
> +                       char *hostname1;
> +                       char *pidstr;
> +                       int index;
> +                       char hostname2[256] = "\0";


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


> +                       gethostname(hostname2, 255);


Why 255 and not 256 ? Moreover, are you sure that this is 0 terminated?


> +                       index = strchr(lockstr, ':') - lockstr;
> +                       if (index > 0) {
> +                               lockstr[index] = '\0';
> +                               hostname1 = lockstr;
> +                               pidstr = lockstr + index + 1;
> +                               pid = atoi(pidstr);
> +                       



- 
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.

Moreover "File is in use" is of course alwayd true, sicne it is 
currently used by this process.

In any case you are missing _(...)

Andreas

--
Prof. Dr. Andreas J. Guelzow
http://www.math.concordia.ab.ca/aguelzow