On 03/22/2012 05:24 PM, Nikola Pajkovsky wrote:
Jiri Moskovcak<[email protected]> 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
- fixed
+ 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
- moved to the top
+ }
+
^ pointless new line before every *else if*
- it's there for a reason because those else if are quite separate
blocks, so I want to make it more obvious
+ 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
- fixed
+ }
+
+ }
+ 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
- according to man pages 0 means success, so anything != 0 isn't OK
+ 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.
- I prefer to set it to 0 before using it, it feels "safer"
+ 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
- fixed
+ 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.
- fixed
+ 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,
};
- ok, changed to:
static const GDBusInterfaceVTable interface_vtable =
{
.method_call = handle_method_call,
.get_property = NULL,
.set_property = 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?
- ok
+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)
- why not? it failed to initialize, so we should inform user about it...
+}
+
+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
- no, it's not:
55: g_timeout = g_timeout_add_seconds(s_timeout, on_timeout_cb, NULL);
+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*
- well, it's a defensive coding, I doesn't break anything, but it could
if we add some code later and forget to add the goto.. so I propose to
leave it there
+ }
+
+out:
+ g_object_unref(auth_result);
+ return result;
+}
comment please, because I haven't run it yet
