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 ("[EMAIL PROTECTED] (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();
_______________________________________________
gnumeric-list mailing list
[email protected]
http://mail.gnome.org/mailman/listinfo/gnumeric-list
