Re: [patch for 1.6.2] dot-file locking

[Tammo Spalink <tspalink telemetry-investments com>]

Thanks a bunch for the comments.

Tammo

Some inline responses:

On Wed, Mar 08, 2006 at 09:58:52PM -0500, Morten Welinder wrote:
> Very interesting.  Some comments:
> 
> 1. Bugzilla is a far better place.  Please open a bug and
>    attach the patch.

Done, I submitted a patch that also includes the changes described in
this mail.

> 2. The guts of this ought to live in goffice, probably go-file.c

Not sure I see this argument.  The "guts" primarily are specialized to
the semantics of gnumeric, doing things like adjusting gui properties.
I agree that probably some of the file operations could move there,
but it seems cleaner to keep it together.

> 3. I don't think the mechanism to lock should be O_EXCL
>     because it does not work with NFS.  Symlinks would
>     work there, but might be an issue on Win32.

O_EXCL is said to (and actually appears to) work fine on modern
kernels (>= 2.6.5).

> 4. Please use g_get_host_name if available.  Alway use g_get_user_name.

Done, see attached incremental patch.

> 5. You need to ensure that other people can read the lock
>     files.

Ah, I suppose you mean setting the umask...  Done.

> 6. If $EVIL creates a 1GB fake lock file, will Gnumeric crash?

Not planning to test this one ;) I was using g_get_file_contents,
which I would hope does not simply crash when files are too big...

The attached new patch explicitly reads only a bounded amount.

> 7. What about character set for the file?  Since the encoding of the username is
>     not known, it appears that we can end up trying to display non-UTF8 text.
>     (Same issue if $EVIL puts random binary stuff into a lockfile.)

New patch should address this.

> 8. You are writing a zero byte at the end of the file.  Worse, it looks like you
>     depend on it being there when you read the file.

Should be fixed now.

> 9. Things need to be translated, i.e., use _() as appropriate.

Done.

> Morten

--- gnumeric-1.6.2/src/workbook-view.h.dotlocking.1	2006-03-09 11:56:04.000000000 -0500
+++ gnumeric-1.6.2/src/workbook-view.h	2006-03-09 11:56:19.000000000 -0500
@@ -49,12 +49,16 @@ typedef struct {
 #define WORKBOOK_VIEW_CLASS(k) (G_TYPE_CHECK_CLASS_CAST ((k), WORKBOOK_VIEW_TYPE, WorkbookViewClass))
 #define IS_WORKBOOK_VIEW(o)    (G_TYPE_CHECK_INSTANCE_TYPE ((o), WORKBOOK_VIEW_TYPE))
 
+/* File Locking */
 typedef enum {
 	WBV_EXT_LOCK_OPEN,
 	WBV_EXT_LOCK_SAVE,
 	WBV_EXT_LOCK_SAVEAS,
 	WBV_SELF_LOCK_FAULT
 } WbvLockNotification;
+void            wb_view_adapt_to_locking (WorkbookView *wbv,
+					  const char *lockdata,
+					  WbvLockNotification ln);
 
 /* Lifecycle */
 GType		 workbook_view_get_type	  (void);
--- gnumeric-1.6.2/src/flock.c.dotlocking.1	2006-03-09 09:05:56.000000000 -0500
+++ gnumeric-1.6.2/src/flock.c	2006-03-09 11:57:33.000000000 -0500
@@ -13,6 +13,7 @@
 #include <glib.h>
 #include <glib/gstdio.h>
 #include <goffice/utils/go-file.h>
+#include <glib/gi18n.h>
 
 #include "gnumeric.h"
 #include "flock.h"
@@ -21,17 +22,16 @@
 #define O_NOFOLLOW (0)
 #endif
 
-const unsigned max_lockdata_sz = 128;
+const unsigned max_lockdata_sz = 1024;
 
-char * gen_lockname (const char *);
-char * gen_lockdata (void);
+static char * gen_lockname (const char *);
+static char * gen_lockdata (void);
+static char * read_lockdata (char *);
 
-char *
+static char *
 gen_lockname (const char *uri)
 {
 	char *dir, *base, *lockname;
-	const char *s0 = "/.";
-	const char *s1 = ".lock";
 
 	if ((uri == NULL) || (strlen(uri) == 0) || (strncmp (uri, "file://", 7) != 0)) {
 		return NULL;
@@ -39,8 +39,7 @@ gen_lockname (const char *uri)
 
 	dir = go_dirname_from_uri (uri, TRUE);
 	base = go_basename_from_uri (uri);
-	lockname = (char *) g_malloc (strlen (dir) + strlen (base) + strlen (s0) + strlen (s1) + 1);
-	sprintf (lockname, "%s%s%s%s", dir, s0, base, s1);
+	lockname = g_strdup_printf ("%s/.%s.lock", dir, base);
 	g_free (base);
 	g_free (dir);
 
@@ -50,37 +49,70 @@ gen_lockname (const char *uri)
 	return lockname;
 }
 
-char *
+#if (! ((GLIB_MAJOR_VERSION > 2) || ((GLIB_MAJOR_VERSION == 2) && (GLIB_MINOR_VERSION >= 8))))
+static const char *
+g_get_host_name (void)
+{
+	static char hostname [HOST_NAME_MAX + 1];
+	
+	if (gethostname (hostname, HOST_NAME_MAX) == -1) {
+		return _("localhost");
+	}
+	return hostname;
+}
+#endif
+
+static char *
 gen_lockdata ()
 {
-	char hostname [HOST_NAME_MAX + 1];
-	struct passwd *pw;
+	const gchar *username;
+	const gchar *hostname;
 	uid_t uid;
 	pid_t pid;
-	char *uid_str, *pid_str;
 	char *data;
-	const char *s0 = "@";
-	const char *s1 = " (uid ";
-	const char *s2 = ", pid ";
-	const char *s3 = ")";
 
-	if (gethostname (hostname, HOST_NAME_MAX) == -1) {
-		return NULL;
-	}
+	username = g_get_user_name();
+	hostname = g_get_host_name();
 	uid = getuid ();
 	pid = getpid ();
-	if ((pw = getpwuid (uid)) == NULL) {
+	data = g_strdup_printf ("%s %s (uid %lu, pid %lu)", username, hostname, 
+				(long unsigned) uid, (long unsigned) pid);
+
+	if (strlen (data) >= max_lockdata_sz) {
+		g_printerr (_("WARNING: lock data size exceeds allowance (%lu >= %lu)\n"), 
+			    strlen (data), max_lockdata_sz);
+	}
+
+	return data;
+}
+
+static char *
+read_lockdata (char *lockname)
+{
+	int fd, r_rc, c_rc;
+	char *data;
+
+	fd = g_open (lockname, O_RDONLY);
+	if (fd == -1) {
+		return NULL;
+	}
+
+	data = (char *) g_malloc0 (max_lockdata_sz);
+	
+	r_rc = read (fd, data, max_lockdata_sz - 1);
+	c_rc = close (fd);
+	
+	/* XXX debugging */
+	/* g_printerr ("lockdata is '%s'\n", data); */
+
+	if ((r_rc == -1) || (c_rc == -1) ||
+	    (! g_utf8_validate(data, -1, NULL))) {
+		g_free (data);
 		return NULL;
 	}
-	uid_str = g_strdup_printf ("%lu", (long unsigned) uid);
-	pid_str = g_strdup_printf ("%lu", (long unsigned) pid);
-	data = (char *) g_malloc (MIN(strlen (pw->pw_name) + strlen (hostname) + strlen (uid_str) + 
-				      strlen (pid_str) + strlen (s0) + strlen (s1) + strlen (s2) + 
-				      strlen (s3) + 1, max_lockdata_sz));
-	snprintf (data, max_lockdata_sz - 1, "%s%s%s%s%s%s%s%s", 
-		  pw->pw_name, s0, hostname, s1, uid_str, s2, pid_str, s3);
-	g_free(uid_str);
-	g_free(pid_str);
+	
+	/* XXX debugging */
+	/* g_printerr ("lockdata is utf8-kosher\n", data); */
 
 	return data;
 }
@@ -91,45 +123,46 @@ flock_acquire (const char* uri)
 	char *lockname = NULL;
 	char *lockdata = NULL;
 	const char *err_msg = NULL;
+	mode_t mask;
 	int fd;
 
 	lockname = gen_lockname (uri);
 	if (lockname == NULL) {
-		err_msg = "illegal lockname";
+		err_msg = _("illegal lockname");
 		goto flock_lock_finish;
 	}
 
 	/* XXX debugging */
 	/* g_printerr ("locking '%s'\n", lockname); */
 
+	mask = umask(0);
 	fd = g_open (lockname, O_WRONLY | O_CREAT | O_EXCL | O_NOFOLLOW, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
+	umask(mask);
  	if (fd != -1) {
 		int w_rc, c_rc;
 
 		lockdata = gen_lockdata ();
 		if (lockdata == NULL) {
-			err_msg = "lock data generation error";
+			err_msg = _("lock data generation error");
 			goto flock_lock_finish;
 		}
 
-		w_rc = write (fd, lockdata, strlen (lockdata) + 1);
+		w_rc = write (fd, lockdata, MIN(strlen (lockdata), max_lockdata_sz));
 		g_free(lockdata);
 		lockdata = NULL;
 		c_rc = close (fd);
 
 		if (w_rc == -1) {
-			err_msg = "lock write failure";
+			err_msg = _("lock write failure");
 			goto flock_lock_finish;
 		}
 		if (c_rc == -1) {
-			err_msg = "lock close failure";
+			err_msg = _("lock close failure");
 			goto flock_lock_finish;
 		}
 	} else if (g_file_error_from_errno(errno) == G_FILE_ERROR_EXIST) {
-		GError *error = NULL;
-		
-		if (! g_file_get_contents (lockname, &lockdata, NULL, &error)) {
-			err_msg = "lock exists, but reading it failed";
+		if ((lockdata = read_lockdata (lockname)) == NULL) {
+			err_msg = _("lock exists, but reading it failed");
 			goto flock_lock_finish;
 		}
 		
@@ -137,7 +170,7 @@ flock_acquire (const char* uri)
 		/* g_printerr ("lock already held by '%s'\n", lockdata); */
 
 	} else {
-		err_msg = "lock open failed";
+		err_msg = _("lock open failed");
 	}
 
   flock_lock_finish: 
@@ -146,7 +179,7 @@ flock_acquire (const char* uri)
 		/* g_printerr ("locking failed for '%s'\n", lockname); */
 		
 		g_assert (lockdata == NULL);
-		lockdata = g_strdup_printf ("unknown (%s)", err_msg);
+		lockdata = g_strdup_printf ("%s (%s)", _("unknown"), err_msg);
 	}
 
 	if (lockname != NULL) {
@@ -162,31 +195,24 @@ flock_verify (const char* uri)
 	char *expected_lockdata = NULL;
 	char *actual_lockdata = NULL;
 	const char *err_msg = NULL;
-	GError *error = NULL;
 	
 	lockname = gen_lockname (uri);
 	if (lockname == NULL) {
-		err_msg = "illegal lockname";
+		err_msg = _("illegal lockname");
 		goto flock_verify_finish;
 	}
 
 	expected_lockdata = gen_lockdata ();
 	if (expected_lockdata == NULL) {
-		err_msg = "lock data generation error";
+		err_msg = _("lock data generation error");
 		goto flock_verify_finish;
 	}
 
 	/* XXX debugging */
 	/* g_printerr ("verifying '%s'\n", lockname); */
 	
-	g_file_get_contents (lockname, &actual_lockdata, NULL, &error);
-	g_assert ((actual_lockdata == NULL && error != NULL) || 
-		  (actual_lockdata != NULL && error == NULL));
-	if (error != NULL) {
-		/* XXX debugging */
-		/* g_printerr ("ERROR: reading lock file -- %s\n", error->message); */
-		g_error_free (error);
-		err_msg = "reading lock failed";
+	if ((actual_lockdata = read_lockdata (lockname)) == NULL) {
+		err_msg = _("reading lock failed");
 		goto flock_verify_finish;
 	}
 
@@ -204,7 +230,7 @@ flock_verify (const char* uri)
 		/* g_printerr ("lock verify failed for '%s'\n", lockname); */
 		
 		g_assert (actual_lockdata == NULL);
-		actual_lockdata = g_strdup_printf ("unknown (%s)", err_msg);
+		actual_lockdata = g_strdup_printf ("%s (%s)", _("unknown"), err_msg);
 	}
 
 	if (lockname != NULL) {
--- gnumeric-1.6.2/src/workbook-view.c.dotlocking.1	2006-03-09 11:00:59.000000000 -0500
+++ gnumeric-1.6.2/src/workbook-view.c	2006-03-09 11:01:40.000000000 -0500
@@ -667,30 +667,30 @@ wb_view_adapt_to_locking (WorkbookView *
 
 	switch (ln) {
 	case WBV_EXT_LOCK_OPEN:
-		raw_msg = "The workbook '%s' is locked, which should mean that "
-			"someone else is currently using it.  The workbook "
-			"will be opened, but saving changes will be disabled "
-			"to prevent the possibility of conflicting changes by "
-			"concurrent users.  Use the SAVE AS menu action to "
-			"store any changes under a new name.  Otherwise try "
-			"re-opening the workbook later.";
+		raw_msg = _("The workbook '%s' is locked, which should mean that "
+			    "someone else is currently using it.  The workbook "
+			    "will be opened, but saving changes will be disabled "
+			    "to prevent the possibility of conflicting changes by "
+			    "concurrent users.  Use the SAVE AS menu action to "
+			    "store any changes under a new name.  Otherwise try "
+			    "re-opening the workbook later.");
 		break;
 	case WBV_EXT_LOCK_SAVE:
-		raw_msg = "Saving of workbook '%s' has been disabled. Use the "
-			"SAVE AS menu action to store any changes under a new name.";
+		raw_msg = _("Saving of workbook '%s' has been disabled. Use the "
+			    "SAVE AS menu action to store any changes under a new name.");
 		break;
 	case WBV_EXT_LOCK_SAVEAS:
-		raw_msg = "The workbook '%s' is locked.  This likely means that "
-			"someone else is currently using it.  To prevent the possibility "
-			"of conflicting changes by concurrent users, saving to this "
-			"workbook has been disabled.";
+		raw_msg = _("The workbook '%s' is locked.  This likely means that "
+			    "someone else is currently using it.  To prevent the possibility "
+			    "of conflicting changes by concurrent users, saving to this "
+			    "workbook has been disabled.");
 		break;
 	case WBV_SELF_LOCK_FAULT:
-		raw_msg = "The lock for workbook '%s' has been removed or damaged.  "
-			"This is unexpected and is probably a sign that something "
-			"has gone wrong.  To prevent the possibility of conflicting "
-			"changes by concurrent users, saving to this workbook has "
-			"been disabled.";
+		raw_msg = _("The lock for workbook '%s' has been removed or damaged.  "
+			    "This is unexpected and is probably a sign that something "
+			    "has gone wrong.  To prevent the possibility of conflicting "
+			    "changes by concurrent users, saving to this workbook has "
+			    "been disabled.");
 		break;
 	default:
 		g_assert_not_reached();