Jiri Moskovcak jmoskovc@redhat.com writes:
+static void handle_method_call(GDBusConnection *connection,
const gchar *caller,
const gchar *object_path,
const gchar *interface_name,
const gchar *method_name,
GVariant *parameters,
GDBusMethodInvocation *invocation,
gpointer user_data)
+{
- reset_timeout();
- uid_t caller_uid;
- GVariant *response;
- if (g_strcmp0(method_name, "GetProblems") == 0)
- {
^ pointless new line
caller_uid = get_caller_uid(connection, invocation, caller);
if (caller_uid == (uid_t) -1)
return;
exctract from all parts of if () caller_uid and check before if's
- }
^ pointless new line before every *else if*
- else if (g_strcmp0(method_name, "GetAllProblems") == 0)
- {
dito
- }
- else if (g_strcmp0(method_name, "ChownProblemDir") == 0)
- {
... ... ...
if (stat(problem_dir, &statbuf) == 0 && S_ISDIR(statbuf.st_mode))
{
if (caller_uid == 0 || uid_in_group(caller_uid, statbuf.st_gid)) //caller seems to be in group with access to this dir, so no action needed
{
//return ok
remove comment
VERB1 log("caller has access to the requested directory %s", problem_dir);
g_dbus_method_invocation_return_value(invocation, NULL);
dd_close(dd);
return;
//free something?
dito comment
}
}
else
{
g_dbus_method_invocation_return_dbus_error(invocation,
"org.freedesktop.problems.StatFailure",
strerror(errno));
dd_close(dd);
return;
}
pwd = getpwuid(caller_uid);
if (pwd)
{
errno = 0;
chown_res = chown(problem_dir, statbuf.st_uid, pwd->pw_gid);
you don't have to set errno to 0, because if chown on success return 0. On error -1 errno is set.
dd_init_next_file(dd);
char *short_name, *full_name;
while (chown_res == 0 && dd_get_next_file(dd, &short_name, &full_name))
{
VERB3 log("chowning %s", full_name);
chown_res = chown(full_name, statbuf.st_uid, pwd->pw_gid);
}
if (chown_res != 0)
according to man-pages, you should have to check -1
g_dbus_method_invocation_return_dbus_error(invocation,
"org.freedesktop.problems.ChownError",
strerror(errno));
g_dbus_method_invocation_return_value(invocation, NULL);
dd_close(dd);
return;
}
VERB3 log("shouldn't get here");
dd_close(dd);
- }
- else if (g_strcmp0(method_name, "GetInfo") == 0)
- {
VERB1 log("GetInfo");
const gchar *problem_dir;
g_variant_get(parameters, "(&s)", &problem_dir);
GVariantBuilder *builder;
struct stat statbuf;
errno = 0;
do not reset errno unless it's needed
On success, zero is returned. On error, -1 is returned, and errno is set appropriately.
if (stat(problem_dir, &statbuf) != 0)
{
g_dbus_method_invocation_return_dbus_error(invocation,
"org.freedesktop.problems.GetInfoError",
strerror(errno));
return;
}
caller_uid = get_caller_uid(connection, invocation, caller);
if (caller_uid == (uid_t) -1)
return;
if (!uid_in_group(caller_uid, statbuf.st_gid))
{
if (polkit_check_authorization_dname(caller, "org.freedesktop.problems.getall") != PolkitYes)
{
VERB1 log("not authorized");
g_dbus_method_invocation_return_dbus_error(invocation,
"org.freedesktop.problems.AuthFailure",
"Not Authorized");
^^^ not translatable some others are not translatable also
return;
}
}
struct dump_dir *dd = dd_opendir(problem_dir, DD_OPEN_READONLY | DD_FAIL_QUIETLY_EACCES);
if (!dd)
{
char *error_msg = g_strdup_printf(_("%s is not a valid problem directory"), problem_dir);
g_dbus_method_invocation_return_dbus_error(invocation,
"org.freedesktop.problems.GetInfo",
error_msg);
free(error_msg);
}
here is a bug, when dd is NULL then you print msg and continue to dd_load_text(dd... to have to exit here.
builder = g_variant_builder_new(G_VARIANT_TYPE_ARRAY);
g_variant_builder_add (builder, "{ss}", g_strdup(FILENAME_TIME), dd_load_text(dd, FILENAME_TIME));
g_variant_builder_add (builder, "{ss}", g_strdup(FILENAME_REASON), dd_load_text(dd, FILENAME_REASON));
char *not_reportable_reason = dd_load_text_ext(dd, FILENAME_NOT_REPORTABLE, 0
| DD_LOAD_TEXT_RETURN_NULL_ON_FAILURE
| DD_FAIL_QUIETLY_ENOENT
| DD_FAIL_QUIETLY_EACCES);
if (not_reportable_reason)
g_variant_builder_add (builder, "{ss}", g_strdup(FILENAME_NOT_REPORTABLE), dd_load_text_ext(dd, FILENAME_NOT_REPORTABLE, 0
| DD_LOAD_TEXT_RETURN_NULL_ON_FAILURE
| DD_FAIL_QUIETLY_ENOENT
| DD_FAIL_QUIETLY_EACCES));
/* the source of the problem:
* - first we try to load component, as we use it on Fedora
*/
char *source = dd_load_text_ext(dd, FILENAME_COMPONENT, 0
| DD_LOAD_TEXT_RETURN_NULL_ON_FAILURE
| DD_FAIL_QUIETLY_ENOENT
| DD_FAIL_QUIETLY_EACCES
);
/* if we don't have component, we fallback to executable */
if (!source)
{
source = dd_load_text_ext(dd, FILENAME_EXECUTABLE, 0
| DD_LOAD_TEXT_RETURN_NULL_ON_FAILURE
| DD_FAIL_QUIETLY_ENOENT
| DD_FAIL_QUIETLY_EACCES
);
}
g_variant_builder_add (builder, "{ss}", g_strdup("source"), source);
char *msg = dd_load_text_ext(dd, FILENAME_REPORTED_TO, 0
| DD_LOAD_TEXT_RETURN_NULL_ON_FAILURE
| DD_FAIL_QUIETLY_ENOENT
| DD_FAIL_QUIETLY_EACCES
);
if (msg)
g_variant_builder_add (builder, "{ss}", g_strdup(FILENAME_REPORTED_TO), msg);
dd_close(dd);
GVariant *response = g_variant_new("(a{ss})", builder);
g_variant_builder_unref(builder);
according to me, you are leaking everywhere here. g_variant_builde_unref only count donw reference counter and you should take care of all returned vaules from load_text and g_strdup (and of course you should have to use xstrdup or check g_strdup to NULL)
g_dbus_method_invocation_return_value(invocation, response);
- }
- else if (g_strcmp0(method_name, "Quit") == 0)
- {
VERB1 log("Quit");
g_dbus_method_invocation_return_value(invocation, NULL);
g_main_loop_quit(loop);
- }
+}
+/* for now */ +static const GDBusInterfaceVTable interface_vtable = +{
- handle_method_call,
- NULL,
- NULL,
+};
this looks like a function. I don't have any strong opinion here, but more readable solution is
static const foo = { .x = bar; };
what is problem here is big or little endians. C doen't guarantee order of list fields. you have to use
static const GDBusInterfaceVTable interface_vtable = { .GDBusInterfaceMethodCallFunc = handle_method_call, .GDBusInterfaceGetPropertyFunc = NULL, .GDBusInterfaceSetPropertyFunc = NULL, };
+static void on_name_acquired (GDBusConnection *connection,
const gchar *name,
gpointer user_data)
+{ +}
e? couldn't be set to NULL at g_bus_own_name call?
+static void on_name_lost (GDBusConnection *connection,
const gchar *name,
gpointer user_data)
+{
- g_print(_("The name '%s' has been lost, please check if other "
"service owning the name is not running.\n"), name);
- exit(1);
daemon should not exit(1)
+}
+int main (int argc, char *argv[]) +{
- /* I18n */
- setlocale(LC_ALL, "");
+#if ENABLE_NLS
- bindtextdomain(PACKAGE, LOCALEDIR);
- textdomain(PACKAGE);
+#endif
- guint owner_id;
- abrt_init(argv);
- const char *program_usage_string = _(
"& [options]"
- );
- enum {
OPT_v = 1 << 0,
OPT_t = 1 << 1,
- };
- /* Keep enum above and order of options below in sync! */
- struct options program_options[] = {
OPT__VERBOSE(&g_verbose),
OPT_INTEGER('t', NULL, &s_timeout, _("Exit after NUM seconds of inactivity")),
OPT_END()
- };
- unsigned opts = parse_opts(argc, argv, program_options, program_usage_string);
- export_abrt_envvars(0);
- /* When dbus daemon starts us, it doesn't set PATH
* (I saw it set only DBUS_STARTER_ADDRESS and DBUS_STARTER_BUS_TYPE).
* In this case, set something sane:
*/
- const char *env_path = getenv("PATH");
- if (!env_path || !env_path[0])
putenv((char*)"PATH=/usr/sbin:/usr/bin:/sbin:/bin");
- msg_prefix = "abrt-dbus"; /* for log(), error_msg() and such */
- if (!(opts & OPT_t))
s_timeout = 120; //if the timeout is not set we default to 120sec
s_timeout is not used; it's write only
+static PolkitResult do_check(PolkitSubject *subject, const char *action_id) +{
... ... ...
- if (polkit_authorization_result_get_is_challenge(auth_result))
- {
/* Can't happen (happens only with
* POLKIT_CHECK_AUTHORIZATION_FLAGS_NONE flag) */
result = PolkitChallenge;
goto out;
- }
- if (polkit_authorization_result_get_is_authorized(auth_result))
- {
result = PolkitYes;
goto out;
pointless goto; you will reach the out after jump off the *if*
- }
+out:
- g_object_unref(auth_result);
- return result;
+}
comment please, because I haven't run it yet