On 11/28/2012 06:39 PM, Jakub Filak wrote:
> +#include <NetworkManager.h>
...
> +static bool is_networking_enabled(void)
> +{
> + GError *error = NULL;
> +
> + /* Create a D-Bus proxy to get the object properties from the NM Manager
> + * object. NM_DBUS_* defines are from NetworkManager.h.
> + */
> + const int flags = G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES
> + | G_DBUS_PROXY_FLAGS_DO_NOT_CONNECT_SIGNALS;
> +
> + GDBusProxy *props_proxy = g_dbus_proxy_new_sync(g_system_bus,
> + flags,
> + NULL /* GDBusInterfaceInfo */,
> + NM_DBUS_SERVICE,
> + NM_DBUS_PATH,
> + DBUS_INTERFACE_PROPERTIES,
> + NULL /* GCancellable */,
> + &error);
Looks like you need NetworkManager.h and NetworkManager-devel
for these two NM_foo defines.
I prefer to not have NetworkManager.h and NetworkManager-devel
dependency. For one, NetworkManager-devel requires NetworkManager,
and I _definitely_ don't want that package on my machine.
IOW: I can't build abrt package on my machine anymore.
The values are:
NM_DBUS_SERVICE = "org.freedesktop.NetworkManager"
NM_DBUS_PATH = "/org/freedesktop/NetworkManager"
NM_DBUS_INTERFACE = "org.freedesktop.NetworkManager"
Can we use them directly?
> + error_msg ("Failed to get NetworkManager proxy: %s", error->message);
I think user is not going to understand this error message.
"Can't connect to NetworkManager over DBus" maybe?
> + error_msg ("Failed to Get State property: %s", error->message);
Same. I propose:
"Can't determine network status via NetworkManager: %s".
> static void fork_exec_gui(void)
> {
> - pid_t pid = vfork();
> + pid_t pid = fork();
> if (pid < 0)
> - perror_msg("vfork");
> + perror_msg("fork");
I don't see why you did vfork -> fork change here.
> if (pid == 0)
> {
> /* child */
> - signal(SIGCHLD, SIG_DFL); /* undo SIG_IGN in abrt-applet */
> -//TODO: pass s_dirs[] as DIR param(s) to abrt-gui
> + /* double fork to avoid GUI zombies */
> + pid_t grand_child = fork();
> + if (grand_child != 0)
> + {
> + if (grand_child < 0) perror_msg("vfork");
Here you can use vfork too.
(If not for some reason I don't see, then message is not consistent)
> + exit(0);
> + }
> +
> + /* grand child */
> + //TODO: pass s_dirs[] as DIR param(s) to abrt-gui
> execl(BIN_DIR"/abrt-gui", "abrt-gui", (char*) NULL);
> /* Did not find abrt-gui in installation directory. Oh well */
> /* Trying to find it in PATH */
> execlp("abrt-gui", "abrt-gui", (char*) NULL);
> perror_msg_and_die("Can't execute '%s'", "abrt-gui");
> }
> +
> + safe_waitpid(pid, /* status */ NULL, /* options */ 0);
> +
What if pid == -1 here?
> +static void run_report_from_applet(const char *dirname)
> +{
> + /* prevent zombies; double fork() */
> + pid_t pid = fork();
> + if (pid < 0)
> + perror_msg("fork()");
> + if (pid == 0)
> + {
> + spawn_event_handler_child(dirname, "report-gui", NULL);
> + exit(0);
> + }
> +
> + safe_waitpid(pid, /* status */ NULL, /* options */ 0);
> +}
Similar in code above.
--
vda
