Corrupt Config On Unsafe Shutdown?

[Brandon McCaig]

Hello,

I just started using cmus yesterday. I am very happy to find a
text-based media player, and impressed just how functionally
complete it is. Thank you to everyone. :)

Anyway, at work I develop Windows-based software (against my
will) and normally run GNU/Linux in a VM for my sanity. I
normally just `shutdown' the VM when I'm done for the day,
expecting things to more or less just gracefully die or recover
the next run.

This morning I was surprised to find that cmus seemed to lock up
as soon as I launched it. Nothing seemed to work. I couldn't
switch windows, I couldn't [q]uit, I couldn't even SIGINT or ^C.
It seemed to load up the library, at least in part, but it seemed
unable to respond to any user inputs.

I'm using the pu branch from git://gitorious.org/cmus/cmus.git.
Currently at e89fd49b9f204eee8529d7631eb2f894b668a95c.

Perplexed, I wondered what could possibly be causing it. I tried
^Q in case I had accidentally hit ^S or something. Still didn't
matter. I superstitiously attempted rebuilding from source, and
checked for new changes as well. As a last resort before asking
for debugging help here I thought that I should remove the
configuration directory and see if that fixes it. That is when I
remembered the unsafe shutdown the evening before (i.e., I
shutdown the system while cmus was still playing in a detached
GNU screen). I suspect then that cmus doesn't care much for being
suddenly killed.

It's a minor inconvenience to have to quit cmus before shutting
down, but is there something that we can do to make the config
saving more reliable e.g., write temporary files and move in one
atomic operation to prevent corruption or something along those
lines?

It seems more sensible to me to save the library as changes are
made to it anyway. For configuration changes, I'd be happy to
:save myself. That is, instead of saving configuration when you
exit. :)

Just looking for some thoughts or RTFMs or something. :) Thanks.

Regards,

[Andrew Fuller]

Post by Brandon McCaig
This morning I was surprised to find that cmus seemed to lock up
as soon as I launched it. Nothing seemed to work. I couldn't
switch windows, I couldn't [q]uit, I couldn't even SIGINT or ^C.
It seemed to load up the library, at least in part, but it seemed
unable to respond to any user inputs.

That bug has been reported several times over the years. The offending
code (options_exit in options.c) seems innocent enough, so I'm not
sure what's causing it. I agree that it seems safer to save the config
every time it changes rather than during program shutdown, but that
still might not solve the root of the bug. I've linked an updated copy
of options.c that overwrites the file atomically, which might be worth
a shot. But the best thing we could get would be a reproducible test
case to properly solve it, and/or the cmus debug log after it occurs.

http://h.qartis.com/cmus/options.c

Andrew

[Gregory Petrosyan]

This patch works around rare bug which leaves autosave file empty after unclean
shutdown — situation that has been reported multiple times on the mailing list.

Signed-off-by: Gregory Petrosyan <***@gmail.com>
---
options.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)


What do you think about this patch?


diff --git a/options.c b/options.c
index 8325c05..f59df93 100644
--- a/options.c
+++ b/options.c
@@ -1158,14 +1158,20 @@ void options_exit(void)
{
struct cmus_opt *opt;
struct filter_entry *filt;
+ char filename_tmp[512];
char filename[512];
FILE *f;
int i;

- snprintf(filename, sizeof(filename), "%s/autosave", cmus_config_dir);
- f = fopen(filename, "w");
+ /*
+ * write all settings to the temporary file and rename it atomically
+ * later to work around once-in-a-blue-moon bug that leaves autosave
+ * file empty, causing much frustration
+ */
+ snprintf(filename_tmp, sizeof(filename_tmp), "%s/.autosave", cmus_config_dir);
+ f = fopen(filename_tmp, "w");
if (f == NULL) {
- warn_errno("creating %s", filename);
+ warn_errno("creating %s", filename_tmp);
return;
}

@@ -1205,6 +1211,11 @@ void options_exit(void)
fprintf(f, "\n");

fclose(f);
+
+ snprintf(filename, sizeof(filename), "%s/autosave", cmus_config_dir);
+ i = rename(filename_tmp, filename);
+ if (i)
+ warn_errno("renaming %s to %s", filename_tmp, filename);
}

struct resume {

[Johannes Weißl]

Hi Gregory,

Post by Gregory Petrosyan
This patch works around rare bug which leaves autosave file empty after unclean
shutdown — situation that has been reported multiple times on the mailing list.

The patch looks good, although I would opt for a "autosave.tmp" instead
of ".autosave" (I don't expect hidden files in hidden directories).
But shouldn't we do that for every file in .cmus, too?

Johannes

[Gregory Petrosyan]

Post by Johannes Weißl

Post by Gregory Petrosyan
This patch works around rare bug which leaves autosave file empty after unclean
shutdown — situation that has been reported multiple times on the mailing list.

The patch looks good, although I would opt for a "autosave.tmp" instead
of ".autosave" (I don't expect hidden files in hidden directories).
But shouldn't we do that for every file in .cmus, too?

Yeah, good idea — I'll do it!

Gregory

[Gregory Petrosyan]

This patch works around rare bug which leaves autosave etc. file empty after
unclean shutdown — situation that has been reported multiple times on the
mailing list.

Signed-off-by: Gregory Petrosyan <***@gmail.com>
---
history.c | 12 ++++++++++--
options.c | 25 +++++++++++++++++++------
2 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/history.c b/history.c
index f541f6d..a06f8e3 100644
--- a/history.c
+++ b/history.c
@@ -21,10 +21,12 @@
#include "file.h"
#include "uchar.h"
#include "list.h"
+#include "prog.h"

#include <sys/types.h>
#include <fcntl.h>
#include <unistd.h>
+#include <stdio.h>

struct history_entry {
struct list_head node;
@@ -81,16 +83,18 @@ void history_load(struct history *history, char *filename, int max_lines)

void history_save(struct history *history)
{
+ char filename_tmp[512];
struct list_head *item;
int fd;
+ ssize_t rc;

- fd = open(history->filename, O_CREAT | O_WRONLY | O_TRUNC, 0666);
+ snprintf(filename_tmp, sizeof(filename_tmp), "%s.tmp", history->filename);
+ fd = open(filename_tmp, O_CREAT | O_WRONLY | O_TRUNC, 0666);
if (fd == -1)
return;
list_for_each(item, &history->head) {
struct history_entry *history_entry;
const char nl = '\n';
- ssize_t rc;

history_entry = list_entry(item, struct history_entry, node);

@@ -104,6 +108,10 @@ void history_save(struct history *history)
}
out:
close(fd);
+
+ rc = rename(filename_tmp, history->filename);
+ if (rc)
+ warn_errno("renaming %s to %s", filename_tmp, history->filename);
}

void history_add_line(struct history *history, const char *line)
diff --git a/options.c b/options.c
index f72b1a2..cf327c7 100644
--- a/options.c
+++ b/options.c
@@ -1305,14 +1305,15 @@ void options_exit(void)
{
struct cmus_opt *opt;
struct filter_entry *filt;
+ char filename_tmp[512];
char filename[512];
FILE *f;
int i;

- snprintf(filename, sizeof(filename), "%s/autosave", cmus_config_dir);
- f = fopen(filename, "w");
+ snprintf(filename_tmp, sizeof(filename_tmp), "%s/autosave.tmp", cmus_config_dir);
+ f = fopen(filename_tmp, "w");
if (f == NULL) {
- warn_errno("creating %s", filename);
+ warn_errno("creating %s", filename_tmp);
return;
}

@@ -1352,6 +1353,11 @@ void options_exit(void)
fprintf(f, "\n");

fclose(f);
+
+ snprintf(filename, sizeof(filename), "%s/autosave", cmus_config_dir);
+ i = rename(filename_tmp, filename);
+ if (i)
+ warn_errno("renaming %s to %s", filename_tmp, filename);
}

struct resume {
@@ -1460,14 +1466,16 @@ void resume_load(void)

void resume_exit(void)
{
+ char filename_tmp[512];
char filename[512];
struct track_info *ti;
FILE *f;
+ int rc;

- snprintf(filename, sizeof(filename), "%s/resume", cmus_config_dir);
- f = fopen(filename, "w");
+ snprintf(filename_tmp, sizeof(filename_tmp), "%s/resume.tmp", cmus_config_dir);
+ f = fopen(filename_tmp, "w");
if (!f) {
- warn_errno("creating %s", filename);
+ warn_errno("creating %s", filename_tmp);
return;
}

@@ -1491,4 +1499,9 @@ void resume_exit(void)
fprintf(f, "browser-dir %s\n", escape(browser_dir));

fclose(f);
+
+ snprintf(filename, sizeof(filename), "%s/resume", cmus_config_dir);
+ rc = rename(filename_tmp, filename);
+ if (rc)
+ warn_errno("renaming %s to %s", filename_tmp, filename);
}

[Johannes Weißl]

Hello Brandon,

Post by Brandon McCaig
It's a minor inconvenience to have to quit cmus before shutting
down, but is there something that we can do to make the config
saving more reliable e.g., write temporary files and move in one
atomic operation to prevent corruption or something along those
lines?

thanks for your detailed bug report! Can you confirm the autosave file
in ~/.cmus was empty/corrupt, or have you removed the directory before
being able to analyze that?

Post by Brandon McCaig
It seems more sensible to me to save the library as changes are
made to it anyway. For configuration changes, I'd be happy to
:save myself. That is, instead of saving configuration when you
exit. :)

Yes, I'd like cmus to autosave for me also! Maybe we can make that
configurable for users that don't want it. I think this was one of the
best changes made to Firefox in the recent years - that it saves all
open tabs persistently all the time. I see how much effort it is
and either post a patch or append it to the wishlist.

Greetings,
Johannes

[Brandon McCaig]

Post by Johannes Weißl
Hello Brandon,

Hello,

Post by Johannes Weißl
thanks for your detailed bug report!

\o/

Post by Johannes Weißl
Can you confirm the autosave file in ~/.cmus was empty/corrupt,
or have you removed the directory before being able to analyze
that?

I rm -fR'd it. :) At the time I only wanted to listen to music. I
can't even seem to reproduce the issue now. I tried kill -9 and
abruptly shuting down, but the config seems fine. My environment
has changed a bit though. I used RPM Fusion to install a
fully-featured cmus (I was having trouble getting it to recognize
m4a files, which I copied over from <evil_media_player>). Now
even when I use my manually compiled cmus it doesn't seem to
corrupt anything upon sudden death. It could just be coincidence,
I guess. :) Maybe the right conditions have to be met for the
corruption to occur. :-/ I'll try again at the end of the day to
see if I can make it happen again...

Regards,

[Johannes Weißl]

Hi Brandon,

Post by Brandon McCaig

Post by Johannes Weißl
Can you confirm the autosave file in ~/.cmus was empty/corrupt,
or have you removed the directory before being able to analyze
that?

I rm -fR'd it. :) At the time I only wanted to listen to music. I
can't even seem to reproduce the issue now.

No problem! I expected it to be very hard to reproduce. Your report was
still useful, because with Gregory's patch we now write the autosave
(and in the future all files) atomically, which (hopefully) removes this
rare bug.

Johannes