On 03/22/2012 05:24 PM, Nikola Pajkovsky wrote:
Jiri Moskovcak<jmoskovc(a)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
- 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