26 Jan
2011
26 Jan
'11
7:24 p.m.
Hi,
After a quick look at abrt-cli source, I think this can be approached like this:
We add -D DIR option which specifies which directory to list. It can be used more than once. If it is not specified, abrt-cli defaults to -D /var/spool/abrt -D $HOME/abrt/spool.
With "abrt-cli --list", dumps from all these directories are shown.
When "abrt-cli --report DUMP_DIR" is run and abrt-cli detects that it can't lock DUMP_DIR for writing, it informs the user:
DUMP_DIR is not writable. Do you want to copy it to $HOME/abrt/spool? (y/n)
(The directory to copy to can be specified by another option, -W DIR; or we can have a convention that the DIR from the first -D DIR option is used)
When "abrt-cli --delete DUMP_DIR" is run and abrt-cli detects that it can't lock DUMP_DIR for writing, it tries to delete the directory through dbus call to abrtd.
I am proceeding with implementing it. But don't hesitate to provide your comments on the above. Maybe I miss some problem?
--
vda
26 Jan
26 Jan
8:15 p.m.
On 01/26/2011 08:24 PM, Denys Vlasenko wrote:
Hi,
After a quick look at abrt-cli source, I think this can be approached like this:
We add -D DIR option which specifies which directory to list. It can be used more than once. If it is not specified, abrt-cli defaults to -D /var/spool/abrt -D $HOME/abrt/spool.
With "abrt-cli --list", dumps from all these directories are shown.
- what are you going to show here? the list of paths or some summary about every dump? if the first one, then user will have hard time guessing which is the dump he wants to process and if the second then you'll hit the locking problem again.. unless you ask the daemon to give you the data...
When "abrt-cli --report DUMP_DIR" is run and abrt-cli detects that it can't lock DUMP_DIR for writing, it informs the user:
DUMP_DIR is not writable. Do you want to copy it to $HOME/abrt/spool? (y/n)
(The directory to copy to can be specified by another option, -W DIR; or we can have a convention that the DIR from the first -D DIR option is used)
- this sounds ok
When "abrt-cli --delete DUMP_DIR" is run and abrt-cli detects that it can't lock DUMP_DIR for writing, it tries to delete the directory through dbus call to abrtd.
- I'm ok with this, but at one point we will have to make the surgery and cut the dbus from daemon (or we can leave it if we can live with the dbus dependency...)
J.
27 Jan
27 Jan
8:56 a.m.
On Wed, 2011-01-26 at 21:15 +0100, Jiri Moskovcak wrote:
On 01/26/2011 08:24 PM, Denys Vlasenko wrote:
Hi,
After a quick look at abrt-cli source, I think this can be approached like this:
We add -D DIR option which specifies which directory to list. It can be used more than once. If it is not specified, abrt-cli defaults to -D /var/spool/abrt -D $HOME/abrt/spool.
With "abrt-cli --list", dumps from all these directories are shown.
- what are you going to show here?
The same thing we already show:
# abrt-cli --list --full Getting crash infos... 0. Crash dump : /var/spool/abrt/ccpp-1294848465-26639 UID : 0 Package : coreutils-8.4-10.fc13 Executable : /bin/sleep Crash Time : Wed 12 Jan 2011 05:07:45 PM CET Crash Count: 4 Hostname : dhcp-25-227.brq.redhat.com 1. Crash dump : /var/spool/abrt/ccpp-1294848466-26657 UID : 0 Package : hdparm-9.27-1.fc13 Executable : /sbin/hdparm Crash Time : Wed 12 Jan 2011 05:07:46 PM CET Crash Count: 1 Hostname : dhcp-25-227.brq.redhat.com
if the second then you'll hit the locking problem again.. unless you ask the daemon to give you the data...
What problem? The dump which can't be locked for writing may be still possible to read. If it can't be read too, then we don't show it.
When "abrt-cli --report DUMP_DIR" is run and abrt-cli detects that it can't lock DUMP_DIR for writing, it informs the user:
DUMP_DIR is not writable. Do you want to copy it to $HOME/abrt/spool? (y/n)
(The directory to copy to can be specified by another option, -W DIR; or we can have a convention that the DIR from the first -D DIR option is used)
- this sounds ok
When "abrt-cli --delete DUMP_DIR" is run and abrt-cli detects that it can't lock DUMP_DIR for writing, it tries to delete the directory through dbus call to abrtd.
- I'm ok with this, but at one point we will have to make the surgery
and cut the dbus from daemon (or we can leave it if we can live with the dbus dependency...)
--
vda
8:57 a.m.
On 01/27/2011 09:56 AM, Denys Vlasenko wrote:
On Wed, 2011-01-26 at 21:15 +0100, Jiri Moskovcak wrote:
On 01/26/2011 08:24 PM, Denys Vlasenko wrote:
Hi,
After a quick look at abrt-cli source, I think this can be approached like this:
We add -D DIR option which specifies which directory to list. It can be used more than once. If it is not specified, abrt-cli defaults to -D /var/spool/abrt -D $HOME/abrt/spool.
With "abrt-cli --list", dumps from all these directories are shown.
- what are you going to show here?
The same thing we already show:
# abrt-cli --list --full Getting crash infos... 0. Crash dump : /var/spool/abrt/ccpp-1294848465-26639 UID : 0 Package : coreutils-8.4-10.fc13 Executable : /bin/sleep Crash Time : Wed 12 Jan 2011 05:07:45 PM CET Crash Count: 4 Hostname : dhcp-25-227.brq.redhat.com
- Crash dump : /var/spool/abrt/ccpp-1294848466-26657 UID : 0 Package : hdparm-9.27-1.fc13 Executable : /sbin/hdparm Crash Time : Wed 12 Jan 2011 05:07:46 PM CET Crash Count: 1 Hostname : dhcp-25-227.brq.redhat.com
if the second then you'll hit the locking problem again.. unless you ask the daemon to give you the data...
What problem? The dump which can't be locked for writing may be still possible to read. If it can't be read too, then we don't show it.
- dd_open() tries to lock the dir anyway, it doesn't know if you want to write or read...
When "abrt-cli --report DUMP_DIR" is run and abrt-cli detects that it can't lock DUMP_DIR for writing, it informs the user:
DUMP_DIR is not writable. Do you want to copy it to $HOME/abrt/spool? (y/n)
(The directory to copy to can be specified by another option, -W DIR; or we can have a convention that the DIR from the first -D DIR option is used)
- this sounds ok
When "abrt-cli --delete DUMP_DIR" is run and abrt-cli detects that it can't lock DUMP_DIR for writing, it tries to delete the directory through dbus call to abrtd.
- I'm ok with this, but at one point we will have to make the surgery
and cut the dbus from daemon (or we can leave it if we can live with the dbus dependency...)
9:18 a.m.
On Thu, 2011-01-27 at 09:57 +0100, Jiri Moskovcak wrote:
On 01/27/2011 09:56 AM, Denys Vlasenko wrote:
On Wed, 2011-01-26 at 21:15 +0100, Jiri Moskovcak wrote:
On 01/26/2011 08:24 PM, Denys Vlasenko wrote:
Hi,
After a quick look at abrt-cli source, I think this can be approached like this:
We add -D DIR option which specifies which directory to list. It can be used more than once. If it is not specified, abrt-cli defaults to -D /var/spool/abrt -D $HOME/abrt/spool.
With "abrt-cli --list", dumps from all these directories are shown.
- what are you going to show here?
The same thing we already show:
# abrt-cli --list --full Getting crash infos... 0. Crash dump : /var/spool/abrt/ccpp-1294848465-26639 UID : 0 Package : coreutils-8.4-10.fc13 Executable : /bin/sleep Crash Time : Wed 12 Jan 2011 05:07:45 PM CET Crash Count: 4 Hostname : dhcp-25-227.brq.redhat.com
- Crash dump : /var/spool/abrt/ccpp-1294848466-26657 UID : 0 Package : hdparm-9.27-1.fc13 Executable : /sbin/hdparm Crash Time : Wed 12 Jan 2011 05:07:46 PM CET Crash Count: 1 Hostname : dhcp-25-227.brq.redhat.com
if the second then you'll hit the locking problem again.. unless you ask the daemon to give you the data...
What problem? The dump which can't be locked for writing may be still possible to read. If it can't be read too, then we don't show it.
- dd_open() tries to lock the dir anyway, it doesn't know if you want to
write or read...
I plan to add a flag, which says "if .lock file cannot de created, read the directory anyway, but save a 'read-only' indicator in struct dump_dir".
--
vda
3:26 p.m.
New subject: [PATCH] abrt-cli conversion to local processing, part 1
Here is the first part of the change.
abrt-cli is converted to process events locally. Only -d operation still goes through the daemon.
On Wed, 2011-01-26 at 20:24 +0100, Denys Vlasenko wrote:
When "abrt-cli --report DUMP_DIR" is run and abrt-cli detects that it can't lock DUMP_DIR for writing, it informs the user:
DUMP_DIR is not writable. Do you want to copy it to $HOME/abrt/spool? (y/n)
(The directory to copy to can be specified by another option, -W DIR; or we can have a convention that the DIR from the first -D DIR option is used)
Stealing of non-writable dump dirs is not implemented yet.
Patch is run-tested.
I will work on implementing stealing atop of this patch.
--
vda
diff -x '*.po' -d -urpN abrt.1/src/cli/CLI.cpp abrt.2/src/cli/CLI.cpp
--- abrt.1/src/cli/CLI.cpp 2011-01-27 12:45:43.709078596 +0100
+++ abrt.2/src/cli/CLI.cpp 2011-01-27 16:16:30.337078297 +0100
@@ -36,6 +36,51 @@ static char *localize_crash_time(const c
return xstrdup(timeloc);
}
+static crash_data_t *FillCrashInfo(const char *dump_dir_name)
+{
+ struct dump_dir *dd = dd_opendir(dump_dir_name, /*flags:*/ 0);
+ if (!dd)
+ return NULL;
+
+ crash_data_t *crash_data = create_crash_data_from_dump_dir(dd);
+// char *events = list_possible_events(dd, NULL, "");
+ dd_close(dd);
+// add_to_crash_data_ext(crash_data, CD_EVENTS, events, CD_FLAG_SYS + CD_FLAG_ISNOTEDITABLE);
+// free(events);
+ add_to_crash_data_ext(crash_data, CD_DUMPDIR, dump_dir_name, CD_FLAG_SYS + CD_FLAG_ISNOTEDITABLE);
+
+ return crash_data;
+}
+
+static void GetCrashInfos(vector_of_crash_data_t *retval, const char *dir_name)
+{
+ VERB1 log("Loading dumps from '%s'", dir_name);
+
+ DIR *dir = opendir(dir_name);
+ if (dir != NULL)
+ {
+ struct dirent *dent;
+ while ((dent = readdir(dir)) != NULL)
+ {
+ if (dot_or_dotdot(dent->d_name))
+ continue; /* skip "." and ".." */
+
+ char *dump_dir_name = concat_path_file(dir_name, dent->d_name);
+
+ struct stat statbuf;
+ if (stat(dump_dir_name, &statbuf) == 0
+ && S_ISDIR(statbuf.st_mode)
+ ) {
+ crash_data_t *crash_data = FillCrashInfo(dump_dir_name);
+ if (crash_data)
+ g_ptr_array_add(retval, crash_data);
+ }
+ free(dump_dir_name);
+ }
+ closedir(dir);
+ }
+}
+
/** Prints basic information about a crash to stdout. */
static void print_crash(crash_data_t *crash_data)
{
@@ -200,35 +245,41 @@ static const char *progname(const char *
* Prints abrt-cli version and some help text.
* Then exits the program with return value 1.
*/
-static void usage(char *argv0)
+static void print_usage_and_die(char *argv0)
{
const char *name = progname(argv0);
printf("%s "VERSION"\n\n", name);
/* Message has embedded tabs. */
- printf(_("Usage: %s [OPTION]\n\n"
- "Startup:\n"
- " -V, --version display the version and exit\n"
- " -v, --verbose increase verbosity\n"
- "Actions:\n"
- " -l, --list print a list of all crashes which are not yet reported\n"
- " -f, --full print a list of all crashes, including the already reported ones\n"
- " -r, --report CRASH_ID create and send a report\n"
- " -y, --always create and send a report without asking\n"
- " -d, --delete CRASH_ID remove a crash\n"
- " -i, --info CRASH_ID print detailed information about a crash\n"
- " -b, --backtrace print detailed information about a crash including backtrace\n"
- "CRASH_ID can be:\n"
- " a name of dump directory, or\n"
- " @N - N'th crash (as displayed by --list --full) will be acted upon\n"
+ printf(_(
+ "Usage: %s -l[f] [-D BASE_DIR]...]\n"
+ " or: %s -r[y] CRASH_DIR\n"
+ " or: %s -i[b] CRASH_DIR\n"
+ " or: %s -d CRASH_DIR\n"
+ "\n"
+ " -l, --list List not yet reported crashes\n"
+ " -f, --full List all crashes\n"
+ " -D BASE_DIR Directory to list crashes from\n"
+ " (default: -D $HOME/abrt/spool -D %s)\n"
+ "\n"
+ " -r, --report Send a report about CRASH_DIR\n"
+ " -y, --always ...without editing and asking\n"
+ " -i, --info Print detailed information about CRASH_DIR\n"
+ " -b, --backtrace ...including backtrace\n"
+ " -d, --delete Remove CRASH_DIR\n"
+ "\n"
+ " -V, --version Display version and exit\n"
+ " -v, --verbose Be verbose\n"
),
- name);
-
+ name, name, name, name,
+ DEBUG_DUMPS_DIR
+ );
exit(1);
}
int main(int argc, char** argv)
{
+ GList *D_list = NULL;
char *dump_dir_name = NULL;
int op = -1;
bool full = false;
@@ -264,12 +315,15 @@ int main(int argc, char** argv)
case 'y': always = true; break;
case 'b': backtrace = true; break;
case 'v': g_verbose++; break;
+ case 'D':
+ D_list = g_list_append(D_list, optarg);
+ break;
case 'V':
printf("%s "VERSION"\n", progname(argv[0]));
return 0;
case '?':
default: /* some error */
- usage(argv[0]); /* exits app */
+ print_usage_and_die(argv[0]); /* exits app */
}
#undef SET_OP
}
@@ -281,15 +335,15 @@ int main(int argc, char** argv)
{
case 0:
if (op == OPT_REPORT || op == OPT_DELETE || op == OPT_INFO)
- usage(argv[0]);
+ print_usage_and_die(argv[0]);
break;
case 1:
if (op != OPT_REPORT && op != OPT_DELETE && op != OPT_INFO)
- usage(argv[0]);
+ print_usage_and_die(argv[0]);
dump_dir_name = argv[optind];
break;
default:
- usage(argv[0]);
+ print_usage_and_die(argv[0]);
}
/* Check if we have an operation.
@@ -300,8 +354,7 @@ int main(int argc, char** argv)
(backtrace && op != OPT_INFO) ||
op == -1)
{
- usage(argv[0]);
- return 1;
+ print_usage_and_die(argv[0]);
}
DBusError err;
@@ -315,7 +368,20 @@ int main(int argc, char** argv)
{
case OPT_GET_LIST:
{
- vector_of_crash_data_t *ci = call_GetCrashInfos();
+ if (!D_list)
+ {
+ char *home = getenv("HOME");
+ if (home)
+ D_list = g_list_append(D_list, concat_path_file(home, "abrt/spool"));
+ D_list = g_list_append(D_list, (void*)DEBUG_DUMPS_DIR);
+ }
+ vector_of_crash_data_t *ci = new_vector_of_crash_data();
+ while (D_list)
+ {
+ char *dir = (char *)D_list->data;
+ GetCrashInfos(ci, dir);
+ D_list = g_list_remove(D_list, dir);
+ }
print_crash_list(ci, full);
free_vector_of_crash_data(ci);
break;
@@ -338,14 +404,16 @@ int main(int argc, char** argv)
}
case OPT_INFO:
{
- int old_logmode = logmode;
- logmode = 0;
-
- crash_data_t *crash_data = call_CreateReport(dump_dir_name);
- if (!crash_data)
- error_msg_and_die("Crash '%s' not found", dump_dir_name);
+ if (run_analyze_event(dump_dir_name) != 0)
+ return 1;
- logmode = old_logmode;
+ /* Load crash_data from (possibly updated by analyze) dump dir */
+ struct dump_dir *dd = dd_opendir(dump_dir_name, /*flags:*/ 0);
+ if (!dd)
+ return -1;
+ crash_data_t *crash_data = create_crash_data_from_dump_dir(dd);
+ dd_close(dd);
+ add_to_crash_data_ext(crash_data, CD_DUMPDIR, dump_dir_name, CD_FLAG_SYS + CD_FLAG_ISNOTEDITABLE);
print_crash_info(crash_data, backtrace);
free_crash_data(crash_data);
diff -x '*.po' -d -urpN abrt.1/src/cli/dbus.cpp abrt.2/src/cli/dbus.cpp
--- abrt.1/src/cli/dbus.cpp 2011-01-20 18:41:37.000000000 +0100
+++ abrt.2/src/cli/dbus.cpp 2011-01-27 15:54:56.970329419 +0100
@@ -121,72 +121,21 @@ static DBusMessage* send_get_reply_and_u
}
}
-vector_of_crash_data_t *call_GetCrashInfos()
-{
- DBusMessage* msg = new_call_msg(__func__ + 5);
- DBusMessage *reply = send_get_reply_and_unref(msg);
-
- DBusMessageIter in_iter;
- dbus_message_iter_init(reply, &in_iter);
-
- vector_of_crash_data_t *argout = NULL;
- int r = load_vector_of_crash_data(&in_iter, &argout);
- if (r != ABRT_DBUS_LAST_FIELD) /* more values present, or bad type */
- error_msg_and_die("dbus call %s: return type mismatch", __func__ + 5);
-
- dbus_message_unref(reply);
- return argout;
-}
-
-crash_data_t *call_CreateReport(const char* crash_id)
-{
- DBusMessage* msg = new_call_msg(__func__ + 5);
- dbus_message_append_args(msg,
- DBUS_TYPE_STRING, &crash_id,
- DBUS_TYPE_INVALID);
-
- DBusMessage *reply = send_get_reply_and_unref(msg);
-
- DBusMessageIter in_iter;
- dbus_message_iter_init(reply, &in_iter);
-
- crash_data_t *argout = NULL;
- int r = load_crash_data(&in_iter, &argout);
- if (r != ABRT_DBUS_LAST_FIELD) /* more values present, or bad type */
- error_msg_and_die("dbus call %s: return type mismatch", __func__ + 5);
-
- dbus_message_unref(reply);
- return argout;
-}
-
-report_status_t call_Report(crash_data_t *report,
- const vector_string_t& reporters,
- GHashTable *plugins)
+void handle_dbus_err(bool error_flag, DBusError *err)
{
- DBusMessage* msg = new_call_msg(__func__ + 5);
- DBusMessageIter out_iter;
- dbus_message_iter_init_append(msg, &out_iter);
-
- /* parameter #1: report data */
- store_crash_data(&out_iter, report);
- /* parameter #2: reporters to use */
- store_val(&out_iter, reporters);
- /* parameter #3 (opt): plugin config */
- if (g_hash_table_size(plugins))
- store_hash_table_map_string_t(&out_iter, plugins);
-
- DBusMessage *reply = send_get_reply_and_unref(msg);
-
- DBusMessageIter in_iter;
- dbus_message_iter_init(reply, &in_iter);
-
- report_status_t result;
- int r = load_val(&in_iter, result);
- if (r != ABRT_DBUS_LAST_FIELD) /* more values present, or bad type */
- error_msg_and_die("dbus call %s: return type mismatch", __func__ + 5);
-
- dbus_message_unref(reply);
- return result;
+ if (dbus_error_is_set(err))
+ {
+ error_msg("dbus error: %s", err->message);
+ /* dbus_error_free(&err); */
+ error_flag = true;
+ }
+ if (!error_flag)
+ return;
+ error_msg_and_die(
+ "error requesting DBus name %s, possible reasons: "
+ "abrt run by non-root; dbus config is incorrect; "
+ "or dbus daemon needs to be restarted to reload dbus config",
+ ABRTD_DBUS_NAME);
}
int32_t call_DeleteDebugDump(const char* crash_id)
@@ -209,74 +158,3 @@ int32_t call_DeleteDebugDump(const char*
dbus_message_unref(reply);
return result;
}
-
-map_map_string_t call_GetPluginsInfo()
-{
- DBusMessage *msg = new_call_msg(__func__ + 5);
- DBusMessage *reply = send_get_reply_and_unref(msg);
-
- DBusMessageIter in_iter;
- dbus_message_iter_init(reply, &in_iter);
-
- map_map_string_t argout;
- int r = load_val(&in_iter, argout);
- if (r != ABRT_DBUS_LAST_FIELD) /* more values present, or bad type */
- error_msg_and_die("dbus call %s: return type mismatch", __func__ + 5);
-
- dbus_message_unref(reply);
- return argout;
-}
-
-map_plugin_settings_t call_GetPluginSettings(const char *name)
-{
- DBusMessage *msg = new_call_msg(__func__ + 5);
- dbus_message_append_args(msg,
- DBUS_TYPE_STRING, &name,
- DBUS_TYPE_INVALID);
-
- DBusMessage *reply = send_get_reply_and_unref(msg);
-
- DBusMessageIter in_iter;
- dbus_message_iter_init(reply, &in_iter);
-
- map_string_t argout;
- int r = load_val(&in_iter, argout);
- if (r != ABRT_DBUS_LAST_FIELD) /* more values present, or bad type */
- error_msg_and_die("dbus call %s: return type mismatch", __func__ + 5);
-
- dbus_message_unref(reply);
- return argout;
-}
-
-map_map_string_t call_GetSettings()
-{
- DBusMessage *msg = new_call_msg(__func__ + 5);
- DBusMessage *reply = send_get_reply_and_unref(msg);
-
- DBusMessageIter in_iter;
- dbus_message_iter_init(reply, &in_iter);
- map_map_string_t argout;
- int r = load_val(&in_iter, argout);
- if (r != ABRT_DBUS_LAST_FIELD) /* more values present, or bad type */
- error_msg_and_die("dbus call %s: return type mismatch", __func__ + 5);
-
- dbus_message_unref(reply);
- return argout;
-}
-
-void handle_dbus_err(bool error_flag, DBusError *err)
-{
- if (dbus_error_is_set(err))
- {
- error_msg("dbus error: %s", err->message);
- /* dbus_error_free(&err); */
- error_flag = true;
- }
- if (!error_flag)
- return;
- error_msg_and_die(
- "error requesting DBus name %s, possible reasons: "
- "abrt run by non-root; dbus config is incorrect; "
- "or dbus daemon needs to be restarted to reload dbus config",
- ABRTD_DBUS_NAME);
-}
diff -x '*.po' -d -urpN abrt.1/src/cli/dbus.h abrt.2/src/cli/dbus.h
--- abrt.1/src/cli/dbus.h 2011-01-20 18:41:37.000000000 +0100
+++ abrt.2/src/cli/dbus.h 2011-01-27 15:54:56.971329138 +0100
@@ -23,46 +23,8 @@
extern DBusConnection* s_dbus_conn;
-vector_of_crash_data_t *call_GetCrashInfos();
-
-crash_data_t *call_CreateReport(const char *crash_id);
-
-/** Sends report using enabled Reporter plugins.
- * @param report
- * The report sent to Reporter plugins.
- * @param reporters
- * List of names of Reporters which should be called.
- * @param plugins
- * Optional settings for Reporter plugins, can be empty.
- * Format: plugins["PluginName"]["SettingsKey"] = "SettingsValue"
- * If it contains settings for some plugin, it must contain _all fields_
- * obtained by call_GetPluginSettings, otherwise the plugin might ignore
- * the settings.
- */
-report_status_t call_Report(crash_data_t *report,
- const vector_string_t& reporters,
- GHashTable *plugins);
-
int32_t call_DeleteDebugDump(const char* crash_id);
-/* Gets basic data about all installed plugins.
- * @todo
- * Return more semantically structured output - maybe a struct instead of a map.
- */
-map_map_string_t call_GetPluginsInfo();
-
-/** Gets default plugin settings.
- * @param name
- * Corresponds to name obtained from call_GetPluginsInfo.
- */
-map_plugin_settings_t call_GetPluginSettings(const char *name);
-
-/** Gets global daemon settings.
- * @todo
- * Return more semantically structured output - maybe a struct instead of a map.
- */
-map_map_string_t call_GetSettings();
-
void handle_dbus_err(bool error_flag, DBusError *err);
#endif
diff -x '*.po' -d -urpN abrt.1/src/cli/report.cpp abrt.2/src/cli/report.cpp
--- abrt.1/src/cli/report.cpp 2011-01-27 15:56:58.470079351 +0100
+++ abrt.2/src/cli/report.cpp 2011-01-27 15:54:56.973330114 +0100
@@ -464,36 +464,51 @@ static bool set_echo(bool enable)
return true;
}
-static void free_map_string_t(gpointer data)
-{
- delete (map_string_t *)data;
-}
/**
* Gets reporter plugin settings.
- * @param reporters
- * List of reporter names. Settings of these reporters are handled.
* @return settings
* A structure filled with reporter plugin settings.
* It's GHashTable<char *, map_plugin_t *> and must be passed to
* g_hash_table_destroy();
*/
-static GHashTable *get_reporter_plugin_settings(const vector_string_t& reporters)
+static void get_plugin_system_setting(GHashTable *settings)
{
- /* First of all, load system-wide report plugin settings. */
- GHashTable *settings = g_hash_table_new_full(g_str_hash, g_str_equal,
- free, free_map_string_t);
+ DIR *dir = opendir(PLUGINS_CONF_DIR);
+ if (!dir)
+ return;
- for (vector_string_t::const_iterator it = reporters.begin(); it != reporters.end(); ++it)
+ struct dirent *dent;
+ while ((dent = readdir(dir)) != NULL)
{
- map_string_t *single_plugin_settings = new map_string_t;
- *single_plugin_settings = call_GetPluginSettings(it->c_str());
+ char *ext = strrchr(dent->d_name, '.');
+ if (!ext || strcmp(ext + 1, "conf") != 0)
+ continue;
+ if (!is_regular_file(dent, PLUGINS_CONF_DIR))
+ continue;
+ VERB3 log("Found %s", dent->d_name);
- // Copy the received settings as defaults.
- // Plugins won't work without it, if some value is missing
- // they use their default values for all fields.
- g_hash_table_replace(settings, xstrdup(it->c_str()), single_plugin_settings);
+ char *conf_file = concat_path_file(PLUGINS_CONF_DIR, dent->d_name);
+ map_string_h *single_plugin_settings = new_map_string();
+ if (load_conf_file(conf_file, single_plugin_settings, /*skip w/o value:*/ false))
+ VERB3 log("Loaded %s", dent->d_name);
+ free(conf_file);
+
+ *ext = '\0';
+ g_hash_table_replace(settings, xstrdup(dent->d_name), single_plugin_settings);
}
+ closedir(dir);
+}
+
+static GHashTable *get_plugin_settings(void)
+{
+ /* First of all, load system-wide plugin settings. */
+ GHashTable *settings = g_hash_table_new_full(
+ g_str_hash, g_str_equal,
+ free, (void (*)(void*))free_map_string
+ );
+
+ get_plugin_system_setting(settings);
/* Second, load user-specific settings, which override
* the system-wide settings. */
@@ -503,7 +518,7 @@ static GHashTable *get_reporter_plugin_s
{
GHashTableIter iter;
char *plugin_name;
- map_string_t *plugin_settings;
+ map_string_h *plugin_settings;
g_hash_table_iter_init(&iter, settings);
while (g_hash_table_iter_next(&iter, (void**)&plugin_name, (void**)&plugin_settings))
{
@@ -528,7 +543,7 @@ static GHashTable *get_reporter_plugin_s
char *value;
g_hash_table_iter_init(&iter2, single_plugin_settings);
while (g_hash_table_iter_next(&iter2, (void**)&key, (void**)&value))
- (*plugin_settings)[key] = xstrdup(value);
+ g_hash_table_replace(plugin_settings, xstrdup(key), xstrdup(value));
free_map_string(single_plugin_settings);
}
@@ -539,13 +554,13 @@ static GHashTable *get_reporter_plugin_s
/**
* Asks user for missing login information
*/
-static void ask_for_missing_settings(const char *plugin_name, map_string_t &single_plugin_settings)
+static void ask_for_missing_settings(const char *plugin_name, map_string_h *single_plugin_settings)
{
// Login information is missing.
- bool loginMissing = single_plugin_settings.find("Login") != single_plugin_settings.end()
- && 0 == strcmp(single_plugin_settings["Login"].c_str(), "");
- bool passwordMissing = single_plugin_settings.find("Password") != single_plugin_settings.end()
- && 0 == strcmp(single_plugin_settings["Password"].c_str(), "");
+ const char *login = get_map_string_item_or_NULL(single_plugin_settings, "Login");
+ const char *password = get_map_string_item_or_NULL(single_plugin_settings, "Password");
+ bool loginMissing = (login && login[0] == '\0');
+ bool passwordMissing = (password && password[0] == '\0');
if (!loginMissing && !passwordMissing)
return;
@@ -555,7 +570,7 @@ static void ask_for_missing_settings(con
if (loginMissing)
{
read_from_stdin(_("Enter your login: "), result, 64);
- single_plugin_settings["Login"] = std::string(result);
+ g_hash_table_replace(single_plugin_settings, xstrdup("Login"), xstrdup(result));
}
if (passwordMissing)
{
@@ -566,77 +581,224 @@ static void ask_for_missing_settings(con
// Newline was not added by pressing Enter because ECHO was disabled, so add it now.
puts("");
- single_plugin_settings["Password"] = std::string(result);
+ g_hash_table_replace(single_plugin_settings, xstrdup("Password"), xstrdup(result));
}
}
-/* Reports the crash over DBus. */
-int report(const char *dump_dir_name, int flags)
+
+struct logging_state {
+ char *last_line;
+};
+static char *do_log_and_save_line(char *log_line, void *param)
{
- // Ask for an initial report.
- crash_data_t *crash_data = call_CreateReport(dump_dir_name);
- if (!crash_data || g_hash_table_size(crash_data) == 0)
+ struct logging_state *l_state = (struct logging_state *)param;
+ log("%s", log_line);
+ free(l_state->last_line);
+ l_state->last_line = log_line;
+ return NULL;
+}
+static int run_events(const char *dump_dir_name,
+ const vector_string_t& events,
+ GHashTable *map_map_settings
+) {
+ int error_cnt = 0;
+ GList *env_list = NULL;
+
+ // Export overridden settings as environment variables
+ GHashTableIter iter;
+ char *plugin_name;
+ map_string_h *single_plugin_settings;
+ g_hash_table_iter_init(&iter, map_map_settings);
+ while (g_hash_table_iter_next(&iter, (void**)&plugin_name, (void**)&single_plugin_settings))
{
- free_crash_data(crash_data);
- return -1;
+ GHashTableIter iter2;
+ char *key;
+ char *value;
+ g_hash_table_iter_init(&iter2, single_plugin_settings);
+ while (g_hash_table_iter_next(&iter2, (void**)&key, (void**)&value))
+ {
+ char *s = xasprintf("%s_%s=%s", plugin_name, key, value);
+ VERB3 log("Exporting '%s'", s);
+ putenv(s);
+ env_list = g_list_append(env_list, s);
+ }
}
- const char *rating_str = get_crash_item_content_or_NULL(crash_data, FILENAME_RATING);
- unsigned rating = rating_str ? xatou(rating_str) : 4;
+ // Run events
+ bool at_least_one_reporter_succeeded = false;
+ std::string message;
+ struct logging_state l_state;
+ l_state.last_line = NULL;
+ struct run_event_state *run_state = new_run_event_state();
+ run_state->logging_callback = do_log_and_save_line;
+ run_state->logging_param = &l_state;
+ for (unsigned i = 0; i < events.size(); i++)
+ {
+ std::string event = events[i];
+
+ int r = run_event_on_dir_name(run_state, dump_dir_name, event.c_str());
+ if (r == -1)
+ {
+ l_state.last_line = xasprintf("Error: no processing is specified for event '%s'", event.c_str());
+ }
+ if (r == 0)
+ {
+ at_least_one_reporter_succeeded = true;
+ printf("%s: %s\n", event.c_str(), (l_state.last_line ? : "Reporting succeeded"));
+ if (message != "")
+ message += ";";
+ message += (l_state.last_line ? : "Reporting succeeded");
+ }
+ else
+ {
+ error_msg("Reporting via '%s' was not successful%s%s",
+ event.c_str(),
+ l_state.last_line ? ": " : "",
+ l_state.last_line ? l_state.last_line : ""
+ );
+ error_cnt++;
+ }
+ free(l_state.last_line);
+ l_state.last_line = NULL;
+ }
+ free_run_event_state(run_state);
+
+ // Unexport overridden settings
+ for (GList *li = env_list; li; li = g_list_next(li))
+ {
+ char *s = (char*)li->data;
+ /* Need to make a copy: just cutting s at '=' and unsetenv'ing
+ * the result would be a bug! s _itself_ is in environment now,
+ * we must not modify it there!
+ */
+ char *name = xstrndup(s, strchrnul(s, '=') - s);
+ VERB3 log("Unexporting '%s'", name);
+ unsetenv(name);
+ free(name);
+ free(s);
+ }
+ g_list_free(env_list);
+
+ // Save reporting results
+ if (at_least_one_reporter_succeeded)
+ {
+ struct dump_dir *dd = dd_opendir(dump_dir_name, /*flags:*/ 0);
+ if (dd)
+ {
+ dd_save_text(dd, FILENAME_MESSAGE, message.c_str());
+ dd_close(dd);
+ }
+ }
+
+ return error_cnt;
+}
+
+
+static char *do_log(char *log_line, void *param)
+{
+ log("%s", log_line);
+ return log_line;
+}
+int run_analyze_event(const char *dump_dir_name)
+{
+ VERB2 log("run_analyze_event('%s')", dump_dir_name);
+
+ struct run_event_state *run_state = new_run_event_state();
+ run_state->logging_callback = do_log;
+ int res = run_event_on_dir_name(run_state, dump_dir_name, "analyze");
+ free_run_event_state(run_state);
+
+ if (res != 0 && res != -1) /* -1 is "nothing was done", here it is ok */
+ {
+ error_msg("Error while running analyze event on '%s'", dump_dir_name);
+ return 1;
+ }
+ return 0;
+}
+
+
+/* Report the crash */
+int report(const char *dump_dir_name, int flags)
+{
+ if (run_analyze_event(dump_dir_name) != 0)
+ return 1;
+
+ /* Load crash_data from (possibly updated by analyze) dump dir */
+ struct dump_dir *dd = dd_opendir(dump_dir_name, /*flags:*/ 0);
+ if (!dd)
+ return -1;
+
+ crash_data_t *crash_data = create_crash_data_from_dump_dir(dd);
+ char *events_as_lines = list_possible_events(dd, NULL, "");
+ dd_close(dd);
- /* Open text editor and give a chance to review the backtrace etc. */
if (!(flags & CLI_REPORT_BATCH))
{
+ /* Open text editor and give a chance to review the backtrace etc */
create_fields_for_editor(crash_data);
int result = run_report_editor(crash_data);
if (result != 0)
{
free_crash_data(crash_data);
- return result;
+ free(events_as_lines);
+ return 1;
+ }
+ /* Save comment, "how to reproduce", backtrace */
+ dd = dd_opendir(dump_dir_name, /*flags:*/ 0);
+ if (dd)
+ {
+//TODO: we should iterate through crash_data and modify all modifiable fields
+ const char *comment = get_crash_item_content_or_NULL(crash_data, FILENAME_COMMENT);
+ const char *reproduce = get_crash_item_content_or_NULL(crash_data, FILENAME_REPRODUCE);
+ const char *backtrace = get_crash_item_content_or_NULL(crash_data, FILENAME_BACKTRACE);
+ if (comment)
+ dd_save_text(dd, FILENAME_COMMENT, comment);
+ if (reproduce)
+ dd_save_text(dd, FILENAME_REPRODUCE, reproduce);
+ if (backtrace)
+ dd_save_text(dd, FILENAME_BACKTRACE, backtrace);
+ dd_close(dd);
}
}
- /* Get possible reporters associated with this particular crash. */
- const char *events = get_crash_item_content_or_NULL(crash_data, CD_EVENTS);
+ /* Get possible reporters associated with this particular crash */
vector_string_t report_events;
- if (events) while (*events)
+ if (events_as_lines)
{
- const char *end = strchrnul(events, '\n');
- if (strncmp(events, "report", 6) == 0
- && (events[6] == '\0' || events[6] == '_')
- ) {
- char *tmp = xstrndup(events, end - events);
- report_events.push_back(tmp);
- free(tmp);
+ char *events = events_as_lines;
+ while (*events)
+ {
+ char *end = strchrnul(events, '\n');
+ if (strncmp(events, "report", 6) == 0
+ && (events[6] == '\0' || events[6] == '_')
+ ) {
+ char *tmp = xstrndup(events, end - events);
+ report_events.push_back(tmp);
+ free(tmp);
+ }
+ events = end;
+ if (!*events)
+ break;
+ events++;
}
- events = end;
- if (!*events)
- break;
- events++;
}
/* Get settings */
- GHashTable *reporters_settings = get_reporter_plugin_settings(report_events);
+ GHashTable *map_map_settings = get_plugin_settings();
int errors = 0;
int plugins = 0;
if (flags & CLI_REPORT_BATCH)
{
puts(_("Reporting..."));
- report_status_t r = call_Report(crash_data, report_events, reporters_settings);
- report_status_t::iterator it = r.begin();
- while (it != r.end())
- {
- vector_string_t &v = it->second;
- printf("%s: %s\n", it->first.c_str(), v[REPORT_STATUS_IDX_MSG].c_str());
- plugins++;
- if (v[REPORT_STATUS_IDX_FLAG] == "0")
- errors++;
- it++;
- }
+ errors += run_events(dump_dir_name, report_events, map_map_settings);
+ plugins += report_events.size();
}
else
{
+ const char *rating_str = get_crash_item_content_or_NULL(crash_data, FILENAME_RATING);
+ unsigned rating = rating_str ? xatou(rating_str) : 4;
+
/* For every reporter, ask if user really wants to report using it. */
for (vector_string_t::const_iterator it = report_events.begin(); it != report_events.end(); ++it)
{
@@ -648,48 +810,42 @@ int report(const char *dump_dir_name, in
continue;
}
- map_string_t *settings = (map_string_t *)g_hash_table_lookup(reporters_settings, it->c_str());
- if (settings)
+//TODO: rethink how we associate report events with configs
+ if (strncmp(it->c_str(), "report_", strlen("report_")) == 0)
{
- map_string_t::iterator rating_setting = settings->find("RatingRequired");
- if (rating_setting != settings->end()
- && string_to_bool(rating_setting->second.c_str())
- && rating < 3)
+ const char *config_name = it->c_str() + strlen("report_");
+ map_string_h *single_plugin_settings = (map_string_h *)g_hash_table_lookup(map_map_settings, config_name);
+ if (single_plugin_settings)
{
- puts(_("Reporting disabled because the backtrace is unusable"));
+ const char *rating_required = get_map_string_item_or_NULL(single_plugin_settings, "RatingRequired");
+ if (rating_required
+ && string_to_bool(rating_required) == true
+ && rating < 3
+ ) {
+ puts(_("Reporting disabled because the backtrace is unusable"));
- const char *package = get_crash_item_content_or_NULL(crash_data, FILENAME_PACKAGE);
- if (package && package[0])
- printf(_("Please try to install debuginfo manually using the command: "debuginfo-install %s" and try again\n"), package);
+ const char *package = get_crash_item_content_or_NULL(crash_data, FILENAME_PACKAGE);
+ if (package && package[0])
+ printf(_("Please try to install debuginfo manually using the command: "debuginfo-install %s" and try again\n"), package);
- plugins++;
- errors++;
- continue;
+ plugins++;
+ errors++;
+ continue;
+ }
+ ask_for_missing_settings(it->c_str(), single_plugin_settings);
}
}
- else
- {
- puts(_("Error loading reporter settings"));
- plugins++;
- errors++;
- continue;
- }
-
- ask_for_missing_settings(it->c_str(), *settings);
vector_string_t cur_event(1, *it);
- report_status_t r = call_Report(crash_data, cur_event, reporters_settings);
- assert(r.size() == 1); /* one reporter --> one report status */
- vector_string_t &v = r.begin()->second;
- printf("%s: %s\n", r.begin()->first.c_str(), v[REPORT_STATUS_IDX_MSG].c_str());
+ errors += run_events(dump_dir_name, cur_event, map_map_settings);
plugins++;
- if (v[REPORT_STATUS_IDX_FLAG] == "0")
- errors++;
}
}
- g_hash_table_destroy(reporters_settings);
- free_crash_data(crash_data);
+ g_hash_table_destroy(map_map_settings);
+
printf(_("Crash reported via %d report events (%d errors)\n"), plugins, errors);
- return errors != 0;
+ free_crash_data(crash_data);
+ free(events_as_lines);
+ return errors;
}
diff -x '*.po' -d -urpN abrt.1/src/cli/report.h abrt.2/src/cli/report.h
--- abrt.1/src/cli/report.h 2011-01-27 15:57:08.579329361 +0100
+++ abrt.2/src/cli/report.h 2011-01-27 15:54:56.974337655 +0100
@@ -18,6 +18,8 @@
#ifndef ABRT_CLI_REPORT_H
#define ABRT_CLI_REPORT_H
+int run_analyze_event(const char *dump_dir_name);
+
/* Report the crash */
enum {
CLI_REPORT_BATCH = 1 << 0,
diff -x '*.po' -d -urpN abrt.1/src/lib/abrt_dbus.h abrt.2/src/lib/abrt_dbus.h
--- abrt.1/src/lib/abrt_dbus.h 2011-01-27 12:10:40.699329072 +0100
+++ abrt.2/src/lib/abrt_dbus.h 2011-01-27 12:11:04.028497264 +0100
@@ -219,34 +219,6 @@ static inline void store_val(DBusMessage
template <typename K, typename V>
static inline void store_val(DBusMessageIter* iter, const std::map<K,V>& val) { store_map(iter, val); }
-/* next patch will rewrite this into c */
-static inline void store_hash_table_map_string_t(DBusMessageIter* iter, GHashTable *ht)
-{
- DBusMessageIter sub_iter;
- if (!dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY, "{sa{ss}}", &sub_iter))
- die_out_of_memory();
-
- GHashTableIter ht_iter;
- gpointer key, value;
-
- g_hash_table_iter_init(&ht_iter, ht);
- while (g_hash_table_iter_next(&ht_iter, &key, &value))
- {
- DBusMessageIter sub_sub_iter;
- if (!dbus_message_iter_open_container(&sub_iter, DBUS_TYPE_DICT_ENTRY, NULL, &sub_sub_iter))
- die_out_of_memory();
-
- store_val(&sub_sub_iter, (char *)key);
- store_val(&sub_sub_iter, *(map_string_t *)value);
-
- if (!dbus_message_iter_close_container(&sub_iter, &sub_sub_iter))
- die_out_of_memory();
- }
-
- if (!dbus_message_iter_close_container(iter, &sub_iter))
- die_out_of_memory();
-}
-
/*
* Helpers for parsing DBus messages
28 Jan
28 Jan
2:31 p.m.
New subject: [PATCH] abrt-cli conversion to local processing, part 2
On Thu, 2011-01-27 at 16:26 +0100, Denys Vlasenko wrote:
Here is the first part of the change.
...
Stealing of non-writable dump dirs is not implemented yet.
Patch is run-tested.
I will work on implementing stealing atop of this patch.
Before stealing can be implemented, we need to fix dump dir locking: creating .lock file in the parent directory is wrong.
For example, we have a race when dump dir is specified by
/dir/dir/dir/symlink_to_dir
name. Just creating /dir/dir/dir/symlink_to_dir.lock is obviously wrong. Currently we use realpath() to resolve the name to a path w/o symlink at the end:
/dir1/dir1/dir1/dir1
and then create /dir1/dir1/dir1/dir1.lock - but this is racy.
Same problem with directories named . and ..
Another problem is that dump dir can be perfectly writable by us, but it's *parent* is not. Currently, we'll fail to lock such dir.
Creating .lock INSIDE dump directory fixes these problems, but makes locking logic somewhat more complex.
Here is the patch which implements that. Please see added comment at the top of dump_dir.c for explanation of new algorithm.
Patch is run-tested.
--
vda
diff -x '*.po' -d -urpN abrt.3/src/lib/dump_dir.c abrt.4/src/lib/dump_dir.c
--- abrt.3/src/lib/dump_dir.c 2011-01-28 15:19:10.508655192 +0100
+++ abrt.4/src/lib/dump_dir.c 2011-01-28 15:14:10.399905156 +0100
@@ -20,11 +20,70 @@
#include "abrtlib.h"
#include "strbuf.h"
-// TODO:
+// Locking logic:
//
-// Perhaps dd_opendir should do some sanity checking like
-// "if there is no "uid" file in the directory, it's not a crash dump",
-// and fail.
+// The directory is locked by creating a symlink named .lock inside it,
+// whose value (where it "points to") is the pid of locking process.
+// We use symlink, not an ordinary file, because symlink creation
+// is an atomic operation.
+//
+// There are two cases where after .lock creation, we might discover
+// that directory is not really free:
+// * another process just created new directory, but didn't manage
+// to lock it before us.
+// * another process is deleting the directory, and we managed to sneak in
+// and create .lock after it deleted all files (including .lock)
+// but before it rmdir'ed the empty directory.
+//
+// Both these cases are detected by the fact that file named "time"
+// is not present (it must be present in any valid dump dir).
+// If after locking the dir we don't see time file, we remove the lock
+// at once and back off. What happens in concurrent processes
+// we interfered with?
+// * "create new dump dir" process just re-tries locking.
+// * "delete dump dir" process just retries rmdir.
+//
+// There is another case when we don't find time file:
+// when the directory is not really a *dump* dir - user gave us
+// an ordinary directory name by mistake.
+// We detect it by bailing out of "lock, check time file; sleep
+// and retry if it doesn't exist" loop using a counter.
+//
+// To make locking work reliably, it's important to set timeouts
+// correctly. For example, dd_create should retry locking
+// its newly-created directory much faster than dd_opendir
+// tries to lock the directory it tries to open.
+
+
+// How long to sleep between "symlink fails with EEXIST,
+// readlink fails with ENOENT" tries. Someone just unlocked the dir.
+// We never bail out in this case, we retry forever.
+// The value can be really small:
+#define SYMLINK_RETRY_USLEEP (10*1000)
+
+// How long to sleep when lock file with valid pid is seen by dd_opendir
+// (we are waiting for other process to unlock or die):
+#define WAIT_FOR_OTHER_PROCESS_USLEEP (500*1000)
+
+// How long to sleep when lock file with valid pid is seen by dd_create
+// (some idiot jumped the gun and locked the dir we just created).
+// Must not be the same as WAIT_FOR_OTHER_PROCESS_USLEEP (we depend on this)
+// and should be small (we have the priority in locking, this is OUR dir):
+#define CREATE_LOCK_USLEEP (10*1000)
+
+// How long to sleep after we locked a dir, found no time file
+// (either we are racing with someone, or it's not a dump dir)
+// and unlocked it;
+// and after how many tries to give up and declare it's not a dump dir:
+#define NO_TIME_FILE_USLEEP (50*1000)
+#define NO_TIME_FILE_COUNT 10
+
+// How long to sleep after we unlocked an empty dir, but then rmdir failed
+// (some idiot jumped the gun and locked the dir we are deleting);
+// and after how many tries to give up:
+#define RMDIR_FAIL_USLEEP (10*1000)
+#define RMDIR_FAIL_COUNT 50
+
static char *load_text_file(const char *path, unsigned flags);
@@ -62,7 +121,8 @@ static int get_and_set_lock(const char*
{
if (errno != EEXIST)
{
- perror_msg("Can't create lock file '%s'", lock_file);
+ if (errno != ENOENT && errno != ENOTDIR)
+ perror_msg("Can't create lock file '%s'", lock_file);
return -1;
}
@@ -73,7 +133,7 @@ static int get_and_set_lock(const char*
if (errno == ENOENT)
{
/* Looks like lock_file was deleted */
- usleep(10 * 1000); /* avoid CPU eating loop */
+ usleep(SYMLINK_RETRY_USLEEP); /* avoid CPU eating loop */
continue;
}
perror_msg("Can't read lock file '%s'", lock_file);
@@ -109,16 +169,21 @@ static int get_and_set_lock(const char*
return 1;
}
-static int dd_lock(struct dump_dir *dd)
+static int dd_lock(struct dump_dir *dd, unsigned sleep_usec)
{
if (dd->locked)
error_msg_and_die("Locking bug on '%s'", dd->dd_dir);
- char lock_buf[strlen(dd->dd_dir) + sizeof(".lock")];
- sprintf(lock_buf, "%s.lock", dd->dd_dir);
-
char pid_buf[sizeof(long)*3 + 2];
sprintf(pid_buf, "%lu", (long)getpid());
+
+ unsigned dirname_len = strlen(dd->dd_dir);
+ char lock_buf[dirname_len + sizeof("/.lock")];
+ strcpy(lock_buf, dd->dd_dir);
+ strcpy(lock_buf + dirname_len, "/.lock");
+
+ unsigned count = NO_TIME_FILE_COUNT;
+ retry:
while (1)
{
int r = get_and_set_lock(lock_buf, pid_buf);
@@ -126,8 +191,34 @@ static int dd_lock(struct dump_dir *dd)
return r; /* error */
if (r > 0)
break; /* locked successfully */
- sleep(1); /* was 0.5 seconds */
+ /* Other process has the lock, wait for it to go away */
+ usleep(sleep_usec);
}
+
+ /* Are we called by dd_opendir (as opposed to dd_create)? */
+ if (sleep_usec == WAIT_FOR_OTHER_PROCESS_USLEEP) /* yes */
+ {
+ strcpy(lock_buf + dirname_len, "/time");
+ if (access(lock_buf, F_OK) != 0)
+ {
+ /* time file doesn't exist. We managed to lock the directory
+ * which was just created by somebody else, or is almost deleted
+ * by delete_file_dir.
+ * Unlock and back off.
+ */
+ strcpy(lock_buf + dirname_len, "/.lock");
+ xunlink(lock_buf);
+ VERB1 log("Unlocked '%s' (no time file)", lock_buf);
+ if (--count == 0)
+ {
+ errno = EISDIR; /* "this is an ordinary dir, not dump dir" */
+ return -1;
+ }
+ usleep(NO_TIME_FILE_USLEEP);
+ goto retry;
+ }
+ }
+
dd->locked = true;
return 0;
}
@@ -137,9 +228,13 @@ static void dd_unlock(struct dump_dir *d
if (dd->locked)
{
dd->locked = 0;
- char lock_buf[strlen(dd->dd_dir) + sizeof(".lock")];
- sprintf(lock_buf, "%s.lock", dd->dd_dir);
+
+ unsigned dirname_len = strlen(dd->dd_dir);
+ char lock_buf[dirname_len + sizeof("/.lock")];
+ strcpy(lock_buf, dd->dd_dir);
+ strcpy(lock_buf + dirname_len, "/.lock");
xunlink(lock_buf);
+
VERB1 log("Unlocked '%s'", lock_buf);
}
}
@@ -185,34 +280,42 @@ struct dump_dir *dd_opendir(const char *
{
struct dump_dir *dd = dd_init();
- /* Used to use rm_trailing_slashes(dir) here, but with dir = "."
- * or "..", or if the last component is a symlink,
- * then lock file is created in the wrong place.
- * IOW: this breaks locking.
- */
- dd->dd_dir = realpath(dir, NULL);
- if (!dd->dd_dir)
- {
- if (!(flags & DD_FAIL_QUIETLY))
- error_msg("'%s' does not exist", dir);
- dd_close(dd);
- return NULL;
- }
- dir = dd->dd_dir;
+ dir = dd->dd_dir = rm_trailing_slashes(dir);
- if (dd_lock(dd) < 0)
+ if (dd_lock(dd, WAIT_FOR_OTHER_PROCESS_USLEEP) < 0)
{
+ if (errno == EISDIR)
+ {
+ /* EISDIR: dd_lock can lock the dir, but it sees no time file there,
+ * even after it retried many times. It must be an ordinary directory!
+ *
+ * Without this check, e.g. abrt-action-print happily prints any current
+ * directory when run without arguments, because its option -d DIR
+ * defaults to "."!
+ */
+ /*if (!(flags & DD_FAIL_QUIETLY))... - no, DD_FAIL_QUIETLY only means
+ * "it's ok if it doesn exist", not "ok if contents is bogus"!
+ */
+ error_msg("'%s' is not a crash dump directory", dir);
+ dd_close(dd);
+ return NULL;
+ }
+
+ if (!(flags & DD_FAIL_QUIETLY)
+ && (errno == ENOENT || errno == ENOTDIR)
+ ) {
+ error_msg("'%s' does not exist", dir);
+ }
dd_close(dd);
return NULL;
}
- struct stat stat_buf;
-
dd->dd_uid = (uid_t)-1L;
dd->dd_gid = (gid_t)-1L;
if (geteuid() == 0)
{
/* In case caller would want to create more files, he'll need uid:gid */
+ struct stat stat_buf;
if (stat(dir, &stat_buf) != 0 || !S_ISDIR(stat_buf.st_mode))
{
if (!(flags & DD_FAIL_QUIETLY))
@@ -224,24 +327,6 @@ struct dump_dir *dd_opendir(const char *
dd->dd_gid = stat_buf.st_gid;
}
- /* Without this check, e.g. abrt-action-print happily prints any current
- * directory when run without arguments, because its option -d DIR
- * defaults to "."! Let's require that at least some crash dump dir
- * specific files exist before we declare open successful:
- */
- char *name = concat_path_file(dir, FILENAME_UID);
- int bad = (lstat(name, &stat_buf) != 0 || !S_ISREG(stat_buf.st_mode));
- free(name);
- if (bad)
- {
- /*if (!(flags & DD_FAIL_QUIETLY))... - no, DD_FAIL_QUIETLY only means
- * "it's ok if it doesn exist", not "ok if contents is bogus"!
- */
- error_msg("'%s' is not a crash dump directory", dir);
- dd_close(dd);
- return NULL;
- }
-
return dd;
}
@@ -289,12 +374,6 @@ struct dump_dir *dd_create(const char *d
return NULL;
}
- if (dd_lock(dd) < 0)
- {
- dd_close(dd);
- return NULL;
- }
-
/* Was creating it with mode 0700 and user as the owner, but this allows
* the user to replace any file in the directory, changing security-sensitive data
* (e.g. "uid", "analyzer", "executable")
@@ -305,6 +384,13 @@ struct dump_dir *dd_create(const char *d
dd_close(dd);
return NULL;
}
+
+ if (dd_lock(dd, CREATE_LOCK_USLEEP) < 0)
+ {
+ dd_close(dd);
+ return NULL;
+ }
+
/* mkdir's mode (above) can be affected by umask, fix it */
if (chmod(dir, 0750) == -1)
{
@@ -365,41 +451,90 @@ struct dump_dir *dd_create(const char *d
return dd;
}
-static void delete_file_dir(const char *dir)
+static int delete_file_dir(const char *dir, bool skip_lock_file)
{
DIR *d = opendir(dir);
if (!d)
- return;
+ {
+ /* The caller expects us to error out only if the directory
+ * still exists (not deleted). If directory
+ * *doesn't exist*, return 0 and clear errno.
+ */
+ if (errno == ENOENT || errno == ENOTDIR)
+ {
+ errno = 0;
+ return 0;
+ }
+ return -1;
+ }
+ bool unlink_lock_file = false;
struct dirent *dent;
while ((dent = readdir(d)) != NULL)
{
if (dot_or_dotdot(dent->d_name))
continue;
+ if (skip_lock_file && strcmp(dent->d_name, ".lock") == 0)
+ {
+ unlink_lock_file = true;
+ continue;
+ }
char *full_path = concat_path_file(dir, dent->d_name);
if (unlink(full_path) == -1 && errno != ENOENT)
{
- if (errno != EISDIR)
+ int err = 0;
+ if (errno == EISDIR)
{
- error_msg("Can't remove '%s'", full_path);
+ errno = 0;
+ err = delete_file_dir(full_path, /*skip_lock_file:*/ false);
+ }
+ if (errno || err)
+ {
+ perror_msg("Can't remove '%s'", full_path);
free(full_path);
closedir(d);
- return;
+ return -1;
}
- delete_file_dir(full_path);
}
free(full_path);
}
closedir(d);
- if (rmdir(dir) == -1)
+
+ /* Here we know for sure that all files/subdirs we found via readdir
+ * were deleted successfully. If rmdir below fails, we assume someone
+ * is racing with us and created a new file.
+ */
+
+ if (unlink_lock_file)
{
- error_msg("Can't remove directory '%s'", dir);
+ char *full_path = concat_path_file(dir, ".lock");
+ xunlink(full_path);
+ free(full_path);
+
+ unsigned cnt = RMDIR_FAIL_COUNT;
+ do {
+ if (rmdir(dir) == 0)
+ return 0;
+ /* Someone locked the dir after unlink, but before rmdir.
+ * This "someone" must be dd_lock().
+ * It detects this (by seeing that there is no time file)
+ * and backs off at once. So we need to just retry rmdir,
+ * with minimal sleep.
+ */
+ usleep(RMDIR_FAIL_USLEEP);
+ } while (--cnt != 0);
}
+
+ int r = rmdir(dir);
+ if (r)
+ perror_msg("Can't remove directory '%s'", dir);
+ return r;
}
void dd_delete(struct dump_dir *dd)
{
- delete_file_dir(dd->dd_dir);
+ delete_file_dir(dd->dd_dir, /*skip_lock_file:*/ true);
+ dd->locked = 0; /* delete_file_dir already removed .lock */
dd_close(dd);
}
1 Feb
1 Feb
12:28 p.m.
New subject: [PATCH] abrt-cli conversion to local processing, part 3
On Fri, 2011-01-28 at 15:31 +0100, Denys Vlasenko wrote:
On Thu, 2011-01-27 at 16:26 +0100, Denys Vlasenko wrote:
Here is the first part of the change.
...
Stealing of non-writable dump dirs is not implemented yet.
Patch is run-tested.
I will work on implementing stealing atop of this patch.
Before stealing can be implemented, we need to fix dump dir locking: creating .lock file in the parent directory is wrong. ... Here is the patch which implements that. Please see added comment at the top of dump_dir.c for explanation of new algorithm.
And here is the third and last part of the conversion. Now abrt-cli -l will show even only readable dump dirs, abrt-clr -r DIR on unwritable DIR will create a copy in $HOME/abrt/spool and then report it. (On writable DIR it is, of course, isn't done).
Lightly run tested.
--
vda
diff -x '*.po' -d -urpN abrt.4/src/cli/CLI.cpp abrt.5/src/cli/CLI.cpp
--- abrt.4/src/cli/CLI.cpp 2011-01-27 16:16:30.337078297 +0100
+++ abrt.5/src/cli/CLI.cpp 2011-02-01 13:10:53.050680697 +0100
@@ -38,7 +38,7 @@ static char *localize_crash_time(const c
static crash_data_t *FillCrashInfo(const char *dump_dir_name)
{
- struct dump_dir *dd = dd_opendir(dump_dir_name, /*flags:*/ 0);
+ struct dump_dir *dd = dd_opendir(dump_dir_name, /*flags:*/ DD_OPEN_READONLY);
if (!dd)
return NULL;
@@ -199,6 +199,46 @@ static void print_crash_info(crash_data_
}
}
+static struct dump_dir *steal_directory(const char *base_dir, const char *dump_dir_name)
+{
+ const char *base_name = strrchr(dump_dir_name, '/');
+ if (base_name)
+ base_name++;
+ else
+ base_name = dump_dir_name;
+
+ struct dump_dir *dd_dst;
+ unsigned count = 100;
+ char *dst_dir_name = concat_path_file(base_dir, base_name);
+ while (1)
+ {
+ dd_dst = dd_create(dst_dir_name, (uid_t)-1);
+ free(dst_dir_name);
+ if (dd_dst)
+ break;
+ if (--count == 0)
+ {
+ error_msg("Can't create new dump dir in '%s'", base_dir);
+ goto ret;
+ }
+ struct timeval tv;
+ gettimeofday(&tv, NULL);
+ dst_dir_name = xasprintf("%s/%s.%u", base_dir, base_name, (int)tv.tv_usec);
+ }
+
+ log("Creating copy in '%s'", dd_dst->dd_dir);
+ if (copy_file_recursive(dump_dir_name, dd_dst->dd_dir) < 0)
+ {
+ /* error. copy_file_recursive already emitted error message */
+ dd_close(dd_dst);
+ xfunc_die();
+ }
+
+ ret:
+ return dd_dst;
+}
+
+
/* Program options */
enum
{
@@ -329,6 +369,14 @@ int main(int argc, char** argv)
}
end_of_arg_parsing: ;
+ if (!D_list)
+ {
+ char *home = getenv("HOME");
+ if (home)
+ D_list = g_list_append(D_list, concat_path_file(home, "abrt/spool"));
+ D_list = g_list_append(D_list, (void*)DEBUG_DUMPS_DIR);
+ }
+
/* Handle option arguments. */
argc -= optind;
switch (argc)
@@ -368,13 +416,6 @@ int main(int argc, char** argv)
{
case OPT_GET_LIST:
{
- if (!D_list)
- {
- char *home = getenv("HOME");
- if (home)
- D_list = g_list_append(D_list, concat_path_file(home, "abrt/spool"));
- D_list = g_list_append(D_list, (void*)DEBUG_DUMPS_DIR);
- }
vector_of_crash_data_t *ci = new_vector_of_crash_data();
while (D_list)
{
@@ -388,6 +429,23 @@ int main(int argc, char** argv)
}
case OPT_REPORT:
{
+ struct dump_dir *dd = dd_opendir(dump_dir_name, DD_OPEN_READONLY);
+ if (!dd)
+ break;
+ int readonly = !dd->locked;
+ dd_close(dd);
+ if (readonly)
+ {
+ log("'%s' is not writable", dump_dir_name);
+ /* D_list can't be NULL here */
+ struct dump_dir *dd_copy = steal_directory((char *)D_list->data, dump_dir_name);
+ if (dd_copy)
+ {
+ dump_dir_name = xstrdup(dd_copy->dd_dir);
+ dd_close(dd_copy);
+ }
+ }
+
exitcode = report(dump_dir_name, (always ? CLI_REPORT_BATCH : 0));
if (exitcode == -1)
error_msg_and_die("Crash '%s' not found", dump_dir_name);
diff -x '*.po' -d -urpN abrt.4/src/daemon/abrt-server.c abrt.5/src/daemon/abrt-server.c
--- abrt.4/src/daemon/abrt-server.c 2011-01-20 21:37:36.000000000 +0100
+++ abrt.5/src/daemon/abrt-server.c 2011-02-01 13:10:53.051678811 +0100
@@ -119,6 +119,7 @@ static void create_debug_dump()
{
error_msg_and_die("Error creating crash dump %s", path);
}
+ dd_create_basic_files(dd, client_uid);
dd_save_text(dd, FILENAME_ANALYZER, analyzer);
dd_save_text(dd, FILENAME_EXECUTABLE, executable);
diff -x '*.po' -d -urpN abrt.4/src/hooks/abrt-hook-ccpp.c abrt.5/src/hooks/abrt-hook-ccpp.c
--- abrt.4/src/hooks/abrt-hook-ccpp.c 2011-01-20 18:41:37.000000000 +0100
+++ abrt.5/src/hooks/abrt-hook-ccpp.c 2011-02-01 13:10:53.052678321 +0100
@@ -497,6 +497,7 @@ int main(int argc, char** argv)
struct dump_dir *dd = dd_create(path, uid);
if (dd)
{
+ dd_create_basic_files(dd, uid);
char *cmdline = get_cmdline(pid); /* never NULL */
char *reason = xasprintf("Process %s was killed by signal %s (SIG%s)", executable, signal_str, signame ? signame : signal_str);
dd_save_text(dd, FILENAME_ANALYZER, "CCpp");
diff -x '*.po' -d -urpN abrt.4/src/include/abrtlib.h abrt.5/src/include/abrtlib.h
--- abrt.4/src/include/abrtlib.h 2011-01-20 18:41:37.000000000 +0100
+++ abrt.5/src/include/abrtlib.h 2011-01-28 18:55:42.105658978 +0100
@@ -123,6 +123,8 @@ off_t copyfd_size(int src_fd, int dst_fd
void copyfd_exact_size(int src_fd, int dst_fd, off_t size);
#define copy_file abrt_copy_file
off_t copy_file(const char *src_name, const char *dst_name, int mode);
+#define copy_file_recursive abrt_copy_file_recursive
+int copy_file_recursive(const char *source, const char *dest);
/* Returns malloc'ed block */
#define encode_base64 abrt_encode_base64
diff -x '*.po' -d -urpN abrt.4/src/include/report/dump_dir.h abrt.5/src/include/report/dump_dir.h
--- abrt.4/src/include/report/dump_dir.h 2011-01-20 18:41:37.000000000 +0100
+++ abrt.5/src/include/report/dump_dir.h 2011-02-01 13:10:53.076678224 +0100
@@ -31,6 +31,7 @@ extern "C" {
enum {
DD_FAIL_QUIETLY = (1 << 0),
+ DD_OPEN_READONLY = (1 << 1),
};
struct dump_dir {
@@ -48,13 +49,14 @@ struct dump_dir *dd_opendir(const char *
* (IOW: if you aren't running under root):
*/
struct dump_dir *dd_create(const char *dir, uid_t uid);
+void dd_create_basic_files(struct dump_dir *dd, uid_t uid);
int dd_exist(struct dump_dir *dd, const char *path);
DIR *dd_init_next_file(struct dump_dir *dd);
int dd_get_next_file(struct dump_dir *dd, char **short_name, char **full_name);
enum {
/* DD_FAIL_QUIETLY bit is valid for dd_load_text_ext too, */
- DD_LOAD_TEXT_RETURN_NULL_ON_FAILURE = (1 << 1),
+ DD_LOAD_TEXT_RETURN_NULL_ON_FAILURE = (1 << 2),
};
char* dd_load_text_ext(const struct dump_dir *dd, const char *name, unsigned flags);
char* dd_load_text(const struct dump_dir *dd, const char *name);
diff -x '*.po' -d -urpN abrt.4/src/lib/copy_file_recursive.c abrt.5/src/lib/copy_file_recursive.c
--- abrt.4/src/lib/copy_file_recursive.c 1970-01-01 01:00:00.000000000 +0100
+++ abrt.5/src/lib/copy_file_recursive.c 2011-02-01 13:10:53.076678224 +0100
@@ -0,0 +1,139 @@
+/*
+ Copyright (C) 2011 ABRT team
+ Copyright (C) 2011 RedHat Inc
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 2 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License along
+ with this program; if not, write to the Free Software Foundation, Inc.,
+ 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+*/
+
+#include "abrtlib.h"
+
+int copy_file_recursive(const char *source, const char *dest)
+{
+ /* This is a recursive function, try to minimize stack usage */
+ /* NB: each struct stat is ~100 bytes */
+ struct stat source_stat;
+ struct stat dest_stat;
+ int retval = 0;
+ int dest_exists = 0;
+
+ if (strcmp(source, ".lock") == 0)
+ goto skip;
+
+ if (stat(source, &source_stat) < 0) {
+ perror_msg("Can't stat '%s'", source);
+ return -1;
+ }
+
+ if (lstat(dest, &dest_stat) < 0) {
+ if (errno != ENOENT) {
+ perror_msg("Can't stat '%s'", dest);
+ return -1;
+ }
+ } else {
+ if (source_stat.st_dev == dest_stat.st_dev
+ && source_stat.st_ino == dest_stat.st_ino
+ ) {
+ error_msg("'%s' and '%s' are the same file", source, dest);
+ return -1;
+ }
+ dest_exists = 1;
+ }
+
+ if (S_ISDIR(source_stat.st_mode)) {
+ DIR *dp;
+ struct dirent *d;
+
+ if (dest_exists) {
+ if (!S_ISDIR(dest_stat.st_mode)) {
+ error_msg("Target '%s' is not a directory", dest);
+ return -1;
+ }
+ /* race here: user can substitute a symlink between
+ * this check and actual creation of files inside dest */
+ } else {
+ /* Create DEST */
+ mode_t mode = source_stat.st_mode;
+ /* Allow owner to access new dir (at least for now) */
+ mode |= S_IRWXU;
+ if (mkdir(dest, mode) < 0) {
+ perror_msg("Can't create directory '%s'", dest);
+ return -1;
+ }
+ }
+ /* Recursively copy files in SOURCE */
+ dp = opendir(source);
+ if (dp == NULL) {
+ retval = -1;
+ goto ret;
+ }
+
+ while (retval == 0 && (d = readdir(dp)) != NULL) {
+ char *new_source, *new_dest;
+
+ if (dot_or_dotdot(d->d_name))
+ continue;
+ new_source = concat_path_file(source, d->d_name);
+ new_dest = concat_path_file(dest, d->d_name);
+ if (copy_file_recursive(new_source, new_dest) < 0)
+ retval = -1;
+ free(new_source);
+ free(new_dest);
+ }
+ closedir(dp);
+
+ goto ret;
+ }
+
+ if (S_ISREG(source_stat.st_mode)) {
+ int src_fd;
+ int dst_fd;
+ mode_t new_mode;
+
+ src_fd = open(source, O_RDONLY);
+ if (src_fd < 0) {
+ perror_msg("Can't open '%s'", source);
+ return -1;
+ }
+
+ /* Do not try to open with weird mode fields */
+ new_mode = source_stat.st_mode;
+
+ // security problem versus (sym)link attacks
+ // dst_fd = open(dest, O_WRONLY|O_CREAT|O_TRUNC, new_mode);
+ /* safe way: */
+ dst_fd = open(dest, O_WRONLY|O_CREAT|O_EXCL, new_mode);
+ if (dst_fd < 0) {
+ close(src_fd);
+ return -1;
+ }
+
+ if (copyfd_eof(src_fd, dst_fd, COPYFD_SPARSE) == -1)
+ retval = -1;
+ close(src_fd);
+ /* Careful: do check that buffered writes succeeded... */
+ if (close(dst_fd) < 0) {
+ perror_msg("Error writing to '%s'", dest);
+ retval = -1;
+ }
+ goto ret;
+ }
+
+ /* Neither dir not regular file: skip */
+
+ skip:
+ log("Skipping '%s'", source);
+ ret:
+ return retval;
+}
diff -x '*.po' -d -urpN abrt.4/src/lib/create_dump_dir.c abrt.5/src/lib/create_dump_dir.c
--- abrt.4/src/lib/create_dump_dir.c 2011-01-20 18:41:37.000000000 +0100
+++ abrt.5/src/lib/create_dump_dir.c 2011-02-01 13:10:53.077678083 +0100
@@ -23,6 +23,8 @@ static struct dump_dir *try_dd_create(co
{
char *path = concat_path_file(base_dir_name, dir_name);
struct dump_dir *dd = dd_create(path, (uid_t)-1L);
+ if (dd)
+ dd_create_basic_files(dd, (uid_t)-1L);
free(path);
return dd;
}
diff -x '*.po' -d -urpN abrt.4/src/lib/dump_dir.c abrt.5/src/lib/dump_dir.c
--- abrt.4/src/lib/dump_dir.c 2011-01-28 16:40:36.336660764 +0100
+++ abrt.5/src/lib/dump_dir.c 2011-02-01 13:19:38.961998348 +0100
@@ -282,8 +282,21 @@ struct dump_dir *dd_opendir(const char *
dir = dd->dd_dir = rm_trailing_slashes(dir);
+ errno = 0;
if (dd_lock(dd, WAIT_FOR_OTHER_PROCESS_USLEEP) < 0)
{
+ if ((flags & DD_OPEN_READONLY) && errno == EACCES)
+ {
+ /* Directory is not writable. If it seems to be readable,
+ * return "read only" dd, not NULL */
+ struct stat stat_buf;
+ if (stat(dir, &stat_buf) == 0
+ && S_ISDIR(stat_buf.st_mode)
+ && access(dir, R_OK) == 0
+ ) {
+ return dd;
+ }
+ }
if (errno == EISDIR)
{
/* EISDIR: dd_lock can lock the dir, but it sees no time file there,
@@ -378,12 +391,31 @@ struct dump_dir *dd_create(const char *d
return NULL;
}
+ bool created_parents = false;
+ try_again:
/* Was creating it with mode 0700 and user as the owner, but this allows
* the user to replace any file in the directory, changing security-sensitive data
* (e.g. "uid", "analyzer", "executable")
*/
if (mkdir(dir, 0750) == -1)
{
+ int err = errno;
+ if (!created_parents && errno == ENOENT)
+ {
+ char *p = dd->dd_dir + 1;
+ while ((p = strchr(p, '/')) != NULL)
+ {
+ *p = '\0';
+ int r = (mkdir(dd->dd_dir, 0755) == 0 || errno == EEXIST);
+ *p++ = '/';
+ if (!r)
+ goto report_err;
+ }
+ created_parents = true;
+ goto try_again;
+ }
+ report_err:
+ errno = err;
perror_msg("Can't create directory '%s'", dir);
dd_close(dd);
return NULL;
@@ -430,12 +462,19 @@ struct dump_dir *dd_create(const char *d
}
}
+ return dd;
+}
+
+void dd_create_basic_files(struct dump_dir *dd, uid_t uid)
+{
char long_str[sizeof(long) * 3 + 2];
time_t t = time(NULL);
sprintf(long_str, "%lu", (long)t);
dd_save_text(dd, FILENAME_TIME, long_str);
+ if (uid == (uid_t)-1)
+ uid = getuid();
sprintf(long_str, "%lu", (long)uid);
dd_save_text(dd, FILENAME_UID, long_str);
@@ -448,11 +487,8 @@ struct dump_dir *dd_create(const char *d
DD_LOAD_TEXT_RETURN_NULL_ON_FAILURE);
if (!release)
release = load_text_file("/etc/redhat-release", /*flags:*/ 0);
- strchrnul(release, '\n')[0] = '\0';
dd_save_text(dd, FILENAME_RELEASE, release);
free(release);
-
- return dd;
}
static int delete_file_dir(const char *dir, bool skip_lock_file)
@@ -608,8 +644,8 @@ static bool save_binary_file(const char
char* dd_load_text_ext(const struct dump_dir *dd, const char *name, unsigned flags)
{
- if (!dd->locked)
- error_msg_and_die("dump_dir is not opened"); /* bug */
+// if (!dd->locked)
+// error_msg_and_die("dump_dir is not opened"); /* bug */
char *full_path = concat_path_file(dd->dd_dir, name);
char *ret = load_text_file(full_path, flags);
@@ -645,8 +681,8 @@ void dd_save_binary(struct dump_dir* dd,
DIR *dd_init_next_file(struct dump_dir *dd)
{
- if (!dd->locked)
- error_msg_and_die("dump_dir is not opened"); /* bug */
+// if (!dd->locked)
+// error_msg_and_die("dump_dir is not opened"); /* bug */
if (dd->next_dir)
closedir(dd->next_dir);
diff -x '*.po' -d -urpN abrt.4/src/lib/Makefile.am abrt.5/src/lib/Makefile.am
--- abrt.4/src/lib/Makefile.am 2011-01-21 00:35:59.000000000 +0100
+++ abrt.5/src/lib/Makefile.am 2011-01-28 18:55:12.824909504 +0100
@@ -20,6 +20,7 @@ libreport_la_SOURCES = \
read_write.c read_write.h \
logging.c logging.h \
copyfd.c \
+ copy_file_recursive.c \
concat_path_file.c \
append_to_malloced_string.c \
overlapping_strcpy.c \
diff -x '*.po' -d -urpN abrt.4/src/plugins/abrt-dump-oops.c abrt.5/src/plugins/abrt-dump-oops.c
--- abrt.4/src/plugins/abrt-dump-oops.c 2011-01-20 20:32:05.000000000 +0100
+++ abrt.5/src/plugins/abrt-dump-oops.c 2011-02-01 13:10:53.079690433 +0100
@@ -506,6 +506,7 @@ static int save_oops_to_dump_dir(GList *
struct dump_dir *dd = dd_create(path, /*uid:*/ 0);
if (dd)
{
+ dd_create_basic_files(dd, /*uid:*/ 0);
dd_save_text(dd, FILENAME_ANALYZER, "Kerneloops");
dd_save_text(dd, FILENAME_EXECUTABLE, "kernel");
dd_save_text(dd, FILENAME_KERNEL, first_line);
1:37 p.m.
New subject: [PATCH] abrt-cli conversion to local processing, part 3
On 02/01/2011 01:28 PM, Denys Vlasenko wrote:
On Fri, 2011-01-28 at 15:31 +0100, Denys Vlasenko wrote:
On Thu, 2011-01-27 at 16:26 +0100, Denys Vlasenko wrote:
Here is the first part of the change.
...
Stealing of non-writable dump dirs is not implemented yet.
Patch is run-tested.
I will work on implementing stealing atop of this patch.
Before stealing can be implemented, we need to fix dump dir locking: creating .lock file in the parent directory is wrong. ... Here is the patch which implements that. Please see added comment at the top of dump_dir.c for explanation of new algorithm.
And here is the third and last part of the conversion. Now abrt-cli -l will show even only readable dump dirs, abrt-clr -r DIR on unwritable DIR will create a copy in $HOME/abrt/spool and then report it. (On writable DIR it is, of course, isn't done).
Lightly run tested.
looks good, put it into git. I will write some stress tests about steeling directory, reading and writing above same directory and so on. Any ideas is welcome.
--
Nikola
3:35 p.m.
New subject: [PATCH] abrt-cli conversion to local processing, part 3
On Tue, 2011-02-01 at 14:37 +0100, Nikola Pajkovsky wrote:
On 02/01/2011 01:28 PM, Denys Vlasenko wrote:
On Fri, 2011-01-28 at 15:31 +0100, Denys Vlasenko wrote:
On Thu, 2011-01-27 at 16:26 +0100, Denys Vlasenko wrote:
Here is the first part of the change.
...
Stealing of non-writable dump dirs is not implemented yet.
Patch is run-tested.
I will work on implementing stealing atop of this patch.
Before stealing can be implemented, we need to fix dump dir locking: creating .lock file in the parent directory is wrong. ... Here is the patch which implements that. Please see added comment at the top of dump_dir.c for explanation of new algorithm.
And here is the third and last part of the conversion. Now abrt-cli -l will show even only readable dump dirs, abrt-clr -r DIR on unwritable DIR will create a copy in $HOME/abrt/spool and then report it. (On writable DIR it is, of course, isn't done).
Lightly run tested.
looks good, put it into git. I will write some stress tests about steeling directory, reading and writing above same directory and so on. Any ideas is welcome.
Thanks, pushed to git.
--
vda
4897
Age (days ago)
4903
Last active (days ago)
crash-catcher@lists.fedorahosted.org
9 comments
3 participants
participants (3)
-
Denys Vlasenko -
Jiri Moskovcak -
Nikola Pajkovsky