Le 05/11/2010 00:27, Lex Trotman a écrit :
> On 5 November 2010 09:37, Colomban Wendling <lists....@herbesfolles.org>
> wrote:
>> Le 04/11/2010 21:42, Dimitar Zhekov a écrit :
>>> On Thu, 4 Nov 2010 17:53:47 +0000
>>> Nick Treleaven <nick.trelea...@btinternet.com> wrote:
>>>
>>>> So it seems that function doesn't handle disk exhaustion safely. (But
>>>> this is no worse than before for Geany).
>>>
>>> Before we had safe or unsafe file saving. Let's keep that choice, shall
>>> we? I can write the patch, it's only a few lines.
>>>
>>> For more than 20 years now, the only safe save is to write the data
>>> into a temporary file in the same directory, and then rename it over
>>> the target file. Even if the rename fails, you still have the temporary
>>> file. That changes the onwership and permissions, which is exactly the
>>> behaviour of the safe g_file_set_contents().
>> Well... stop me if I'm saying bullshit but what about:
>>
>> backup = path + '~' # or whatever
>> if copy (path, backup_path): # COPY file to the backup
>> if not write (path, data): # write in original file
>> move (backup_path, path) # if write failed, restore backup
>> elif not make_backup:
>> unlink (backup_path) # if no backup is to be done, delete backup
>>
>> The problems I see are:
>> 1) needs at least 2 times the size of the file in the target directory
>> (but it's the same anytime we bake backups)
>> 2) backup file may have altered permissions/attrs
>> 3) ...so if write fails, we have changed the permissions/attrs on the
>> original file
>> In the worst case, the original file has another name and lost its
>> permissions/attrs.
Hum, I found another problem (but seems to apply more or less to every
safe-file-saving methods): write access to the directory containing the
file to save is needed (to create the backup or restore it if write failed).
>> The advantages I see are:
>> 1) the permissions/attrs are kept on success, as well as hard links
>> 2) no data can be lost (if the FS is reliable)
>>
>> The key idea is not to move the original file but to copy it. So we can
>> safely overwrite the original (and the keep permissions/attrs), once the
>> backup is done.
>>
>> Thoughts?
>
>
> Hi,
>
> It seems to me that g_file_set_contents behavior can be described as
> "write new file with the name of an existing file" so new file
> permissions and hard links point to the old file.
>
> Columbans suggestion can be described as "update the contents of an
> existing file" so old permissions and hard links still point to that
> file.
Note that even if it has problems, g_file_replace_contents() seems to
keep the attributes quite correctly.
> "Update the contents" seems much closer to the expected behavior of an
> editor than does "write a new file", so its good from that point of
> view.
>
> But from an efficiency point of view its much more work. Probably not
> a problem on a local filesystem, but on a remote filesystem it
> requires three transfers of the data instead of one, read the old file
> and write the backup then write the new file. As I only use remote
> filesystems on fast networks I can't say how important this is likely
> to be, but for big files/lots of files it may be a problem, also I'm
> thinking web sites edited via ftp, ssh etc. This seems to be the use
> case for most of the performance complaints.
Well, right. I didn't thought that copy is unlikely to be done by the
remote machine (is a filesystem/GVFS-backends clever enough to support
copies?)...
So yes, it would probably be a significant overhead on non-local files.
Needs some testing probably.
> If making a backup file is selected when the file is opened then the
> write of the backup happens then, uses the data read for the open and
> doesn't need to happen at save time, it could even be asynchronous so
> long as it was checked for correct completion before saving. This
> would reduce the user visible performance impact.
Theoretically yes, but I would not do that because another application
may have changed the file since we loaded it and we may not have noticed.
I think this looses a bit of the usefulness of a backup.
And it would also create a backup for each and every file that gets
opened in Geany, not only the ones that gets saved, so.
> Personally I would implement both and let the user choose (especially
> as g_file_set_contents exists).
Why not. Fast & reliable v.s. even more reliable.
But it needs to write two different code paths, and if we want to
support direct GIO API (which is probably a good idea IMHO, at least for
the GNOME desktop with remote FS) as well as standard C API, we need
about 4 code paths.
I join here a small program I wrote to test the idea, you can test it
and see if you find problems. Note that this isn't well tested, but
still, seems to work fine (both in the idea and in the tests I've done
-- basically only no space left and normal).
To switch between plain C/GIO, change the value of the USE_GIO constant
on the top of the file.
Regards,
Colomban
#include <stdio.h>
#include <glib.h>
#define USE_GIO 0
/*
* The idea of this "safe file saving" is:
*
* 1) copy the original file to a backup
* 2) if succeeded, write the new data in the original file
* 3) if succeeded, delete the backup file unless we want bakcups
* 4) if write failed, rename the backup to the original name
*
* This allows to keep file attributes (permissions, etc.) and hard links on
* most cases: only a failure at one of the steps would potentially lost
* these -- but not the data.
* In the worst case, we can end up with a backup file that has lost its
* attributes and that has teh baksup name (filename~ or whatever).
*
* Some drawbacks:
* - We need to *copy* (and not move) the original file to the backup. This is
* likely to need to read and write the whole file, so to transfer the data.
* On a local file system, this is probably reasonable, but on some remote
* mounts it may be really slow.
* - The directory containing the file to save must be writable
* - ...
*/
/*
gcc -o safe-file-saving safe-file-saving.c \
$(pkg-config --libs --cflags glib-2.0 gio-2.0) \
-W -Wall -g $CFLAGS
*/
#if USE_GIO
#include <gio/gio.h>
/* GIO */
/*
* writes @data into file, removing old data.
*
* Using g_file_open_readwrite() is not really the better thing to do but GIO
* doesn't provide a raw "empty and open for writing" command; and we won't use
* g_file_replace_data() or _replace() since we are re-implementing the safe
* saving strategy.
*/
static gboolean
file_write_gio (GFile *file,
const gchar *data,
gsize len,
GError **error)
{
GFileIOStream *iostream;
gboolean success = FALSE;
iostream = g_file_open_readwrite (file, NULL, error);
if (iostream) {
if (g_seekable_truncate (G_SEEKABLE (iostream), G_GOFFSET_CONSTANT (0),
NULL, error)) {
GOutputStream *ostream;
ostream = g_io_stream_get_output_stream (G_IO_STREAM (iostream));
success = g_output_stream_write_all (ostream, data, len, NULL, NULL,
error);
}
g_object_unref (iostream);
}
return success;
}
static gboolean
safe_file_save_gio (const gchar *filename,
const gchar *data,
gsize len,
gboolean make_backup,
GError **error)
{
GFile *file;
GFile *backup;
gchar *backup_filename;
gboolean success = FALSE;
backup_filename = g_strconcat (filename, "~", NULL);
file = g_file_new_for_path (filename);
backup = g_file_new_for_path (backup_filename);
if (! g_file_copy (file, backup,
G_FILE_COPY_ALL_METADATA | G_FILE_COPY_OVERWRITE,
NULL, NULL, NULL, error)) {
/* try to make sure we don't leave a partial file around */
g_file_delete (backup, NULL, NULL);
} else {
if (! file_write_gio (file, data, len, error)) {
/* try to restore the original file, but don't handle errors since we
* already have one */
g_file_move (backup, file,
G_FILE_COPY_ALL_METADATA | G_FILE_COPY_OVERWRITE,
NULL, NULL, NULL, NULL);
} else {
if (! make_backup) {
success = g_file_delete (backup, NULL, error);
} else {
success = TRUE;
}
}
}
g_object_unref (backup);
g_object_unref (file);
g_free (backup_filename);
return success;
}
#define safe_file_save safe_file_save_gio
#else /* USE_GIO */
#include <errno.h>
#include <glib/gstdio.h>
/* no GIO */
/* Some wrappers around C calls to set a GError on failure */
static FILE *
file_open_posix (const gchar *path,
const gchar *modes,
GError **error)
{
FILE *fp;
fp = fopen (path, modes);
if (! fp) {
gint errnum = errno;
g_set_error (error, G_FILE_ERROR, g_file_error_from_errno (errnum),
"Failed to open file \"%s\": %s", path, g_strerror (errnum));
}
return fp;
}
static gboolean
file_write_posix (FILE *fp,
const gchar *data,
gsize len,
GError **error)
{
gboolean success = FALSE;
if (fwrite (data, sizeof (*data), len, fp) != len) {
gint errnum = errno;
g_set_error (error, G_FILE_ERROR, g_file_error_from_errno (errnum),
"Failed to write %lu bytes: %s", len, g_strerror (errnum));
} else {
success = TRUE;
}
return success;
}
static gboolean
file_read_posix (FILE *fp,
gchar *buf,
gsize len,
gsize *read_len,
GError **error)
{
gboolean success = FALSE;
*read_len = fread (buf, sizeof (*buf), len, fp);
if (ferror (fp)) {
gint errnum = errno;
g_set_error (error, G_FILE_ERROR, g_file_error_from_errno (errnum),
"Failed to read file %lu bytes: %s", len, g_strerror (errnum));
} else {
success = TRUE;
}
return success;
}
static gboolean
path_unlink_posix (const gchar *path,
GError **error)
{
gboolean success;
success = g_unlink (path) == 0;
if (! success) {
gint errnum = errno;
g_set_error (error, G_FILE_ERROR, g_file_error_from_errno (errnum),
"Failed to unlink file \"%s\": %s", path, g_strerror (errnum));
}
return success;
}
/* actual copy implementation */
static gboolean
path_write_posix (const gchar *path,
const gchar *data,
gsize len,
GError **error)
{
FILE *fp;
gboolean success = FALSE;
fp = file_open_posix (path, "w", error);
if (fp) {
success = file_write_posix (fp, data, len, error);
}
return success;
}
static gboolean
path_copy_posix (const gchar *src,
const gchar *dst,
GError **error)
{
FILE *fp_src;
FILE *fp_dst;
gboolean success = FALSE;
fp_src = file_open_posix (src, "r", error);
if (fp_src) {
fp_dst = file_open_posix (dst, "w", error);
if (fp_dst) {
gchar buf[BUFSIZ];
gsize read_len;
do {
success = file_read_posix (fp_src, buf, sizeof (buf), &read_len, error);
if (success && read_len > 0) {
success = file_write_posix (fp_dst, buf, read_len, error);
}
} while (success && read_len >= sizeof (buf));
/* if we created the copy but failed to write it, remove it not to leave
* a partial file around */
if (! success) {
g_unlink (dst);
}
fclose (fp_dst);
}
fclose (fp_src);
}
return success;
}
static gboolean
safe_file_save_posix (const gchar *filename,
const gchar *data,
gsize len,
gboolean make_backup,
GError **error)
{
gboolean success = FALSE;
gchar *backup_filename;
backup_filename = g_strconcat (filename, "~", NULL);
if (path_copy_posix (filename, backup_filename, error)) {
if (! path_write_posix (filename, data, len, error)) {
/* try to restore the original file, but don't handle errors since we
* already have one */
g_rename (backup_filename, filename);
} else {
if (! make_backup) {
success = path_unlink_posix (backup_filename, error);
} else {
success = TRUE;
}
}
}
g_free (backup_filename);
return success;
}
#define safe_file_save safe_file_save_posix
#endif /* USE_GIO */
/* test program */
int
main (int argc,
char **argv)
{
gint ret = 1;
#if USE_GIO
g_type_init ();
#endif
if (argc < 2) {
fprintf (stderr, "USAGE: %s FILE [DATA...]", argv[0]);
} else {
const gchar *filename = argv[1];
GString *data;
GError *err = NULL;
data = g_string_new (NULL);
if (argc > 2) {
/* use CLI args as file's content */
gint i;
for (i = 2; i < argc; i++) {
g_string_append (data, argv[i]);
if (i + 1 < argc) {
g_string_append_c (data, ' ');
}
}
} else {
/* use data read from stdin as file's content */
gchar buf[BUFSIZ];
gsize read_len;
do {
if ((read_len = fread (buf, sizeof (*buf), sizeof (buf), stdin)) > 0) {
g_string_append_len (data, buf, (gssize)read_len);
}
} while (read_len >= sizeof (buf));
}
if (! safe_file_save (filename, data->str, data->len, FALSE, &err)) {
g_warning ("Failed to save file: %s", err->message);
g_error_free (err);
} else {
ret = 0;
}
g_string_free (data, TRUE);
}
return ret;
}
_______________________________________________
Geany-devel mailing list
Geany-devel@uvena.de
http://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel
