GNOME Bugzilla – Bug 697719
add full support for OpenBSD
Last modified: 2015-03-29 12:22:58 UTC
Created attachment 241159 [details] [review] add support for OpenBSD Hi. This patch adds support for tracker-control and friends on OpenBSD where /proc does not exist. I am aware that it's not a trivial patch and that there are some ifdef... but I was trying to prevent code duplication when possible. We have been using some similar patches in our ports tree for more than 2 years without issue. * before $ tracker-control Could not open /proc, Error opening directory '/proc': No such file or directory Found 0 PIDs? Could not stat() file:'/proc/1554/cmdline', Error when getting information for file '/proc/1554/cmdline': No such file or directory Store: * after $ tracker-control Found process ID 26753 for 'tracker-store' Store: 10 Apr 2013, 16:09:40: ? Store - Idle Miners: 10 Apr 2013, 16:09:40: ? File System - Not running or is a disabled plugin 10 Apr 2013, 16:09:40: ? Applications - Not running or is a disabled plugin Thoughts?
Created attachment 241164 [details] [review] add support for OpenBSD Simplify: remove a non needed ifndef.
Comment on attachment 241164 [details] [review] add support for OpenBSD >@@ -23,6 +23,14 @@ > #include <gio/gunixinputstream.h> > #include <gio/gunixoutputstream.h> > >+#ifdef __OpenBSD__ >+#include <sys/param.h> >+#include <sys/proc.h> >+#include <sys/sysctl.h> >+#include <fcntl.h> >+#include <kvm.h> >+#endif >+ > #include "tracker-dbus.h" > #include "tracker-log.h" > >@@ -139,6 +147,7 @@ client_data_new (gchar *sender) > } > > if (get_binary) { >+#ifndef __OpenBSD__ > gchar *filename; > gchar *pid_str; > gchar *contents = NULL; >@@ -171,6 +180,29 @@ client_data_new (gchar *sender) > > g_strfreev (strv); > g_free (contents); >+#else >+ gint nproc; >+ struct kinfo_proc *kp; >+ kvm_t *kd; >+ gchar **strv; >+ >+ if ((kd = kvm_openfiles (NULL, NULL, NULL, KVM_NO_FILES, NULL)) == NULL) >+ return cd; >+ >+ if ((kp = kvm_getprocs (kd, KERN_PROC_PID, cd->pid, sizeof (*kp), &nproc)) == NULL) >+ goto out; >+ >+ if ((kp->p_flag & P_SYSTEM) != 0) >+ goto out; >+ >+ if ((strv = kvm_getargv (kd, kp, 0)) == NULL) >+ goto out; I prefer not to use goto statements in our code. It would be much cleaner to have a series of conditions and a logging message for cases which fail unexpectedly, e.g. g_warning ("Could not get process name due to kvm_openfiles() failure"); >+ >+ cd->binary = g_path_get_basename (strv[0]); I am guessing strv here is not owned by us and freed with kvm_close() right? >diff --git a/src/tracker-control/tracker-control-general.c b/src/tracker-control/tracker-control-general.c >index 07231ef..af741ae 100644 >--- a/src/tracker-control/tracker-control-general.c >+++ b/src/tracker-control/tracker-control-general.c >@@ -21,6 +21,15 @@ > > #include <errno.h> > >+#ifdef __OpenBSD__ >+#include <sys/param.h> >+#include <sys/proc.h> >+#include <sys/sysctl.h> >+#include <fcntl.h> >+#include <limits.h> >+#include <kvm.h> >+#endif >+ > #include <glib.h> > #include <glib/gi18n.h> > >@@ -645,6 +654,7 @@ tracker_control_general_run (void) > if (kill_option != TERM_NONE || > terminate_option != TERM_NONE || > list_processes) { >+#ifndef __OpenBSD__ > guint32 own_pid; > guint32 own_uid; > gchar *own_pid_str; >@@ -695,6 +705,29 @@ tracker_control_general_run (void) > strv = g_strsplit (contents, "^@", 2); > if (strv && strv[0]) { > gchar *basename; >+#else /* __OpenBSD__ */ >+ gchar *basename, **strv; >+ int i, nproc; >+ struct kinfo_proc *plist, *kp; >+ char buf[_POSIX2_LINE_MAX]; >+ kvm_t *kd; >+ >+ if ((kd = kvm_openfiles (NULL, NULL, NULL, KVM_NO_FILES, buf)) == NULL) { >+ printf ("%s\n", buf); >+ return EXIT_FAILURE; >+ } >+ >+ plist = kvm_getprocs (kd, KERN_PROC_ALL, 0, sizeof (*plist), &nproc); >+ if (plist == NULL) >+ return EXIT_FAILURE; >+ >+ for (i = 0, kp = plist; i < nproc; i++, kp++) { >+ if ((kp->p_flag & P_SYSTEM) != 0) >+ continue; >+ >+ if ((strv = kvm_getargv (kd, kp, 0)) == NULL) >+ continue; >+#endif Generally speaking, I think this is quite messy. I would much prefer it was abstracted away so we didn't need this in the code actually handling process control. OR at the very least abstracted away into a function where possible to keep this code clean. > > basename = g_path_get_basename (strv[0]); > >@@ -704,7 +737,11 @@ tracker_control_general_run (void) > g_str_has_suffix (basename, "-status-icon") == FALSE) { > pid_t pid; > >+#ifndef __OpenBSD__ > pid = atoi (l->data); >+#else >+ pid = kp->p_pid; >+#endif > str = g_strdup_printf (_("Found process ID %d for '%s'"), pid, basename); > g_print ("%s\n", str); > g_free (str); >@@ -757,6 +794,7 @@ tracker_control_general_run (void) > g_free (basename); > } > >+#ifndef __OpenBSD__ > g_strfreev (strv); > g_free (contents); > g_free (filename); >@@ -764,6 +802,7 @@ tracker_control_general_run (void) > > g_slist_foreach (pids, (GFunc) g_free, NULL); > g_slist_free (pids); >+#endif > > /* If we just wanted to list processes, all done */ > if (list_processes && Overall the patch looks quite good, but just has some nitpicks IMO to fix. Thanks for the patch!
Hi Martyn. Thanks for the review. I decided to first concentrate on the first patch (dbus) and once this gets committed, I'll come up with something cleaner for tracker-control. <snip> > I prefer not to use goto statements in our code. It would be much cleaner to > have a series of conditions and a logging message for cases which fail > unexpectedly, e.g. g_warning ("Could not get process name due to > kvm_openfiles() failure"); I removed the goto and rewrote some parts. It should be cleaner and we display the error now. > > >+ > >+ cd->binary = g_path_get_basename (strv[0]); > > I am guessing strv here is not owned by us and freed with kvm_close() right? This is correct, quoting the man page: "The memory allocated to the argv pointers and string storage is owned by the kvm(3) library. Subsequent kvm_getprocs() and kvm_close(3) calls will clobber this storage." > Overall the patch looks quite good, but just has some nitpicks IMO to fix. > Thanks for the patch! Sure thing. New patch attached, hopefully this is what you had in mind.
Created attachment 241823 [details] [review] implement tracker-dbus on OpenBSD
Comment on attachment 241823 [details] [review] implement tracker-dbus on OpenBSD Thanks for the patch.
(In reply to comment #5) > (From update of attachment 241823 [details] [review]) > Thanks for the patch. Thanks Martyn, it's in: 67ccf1dbda724991d1bf1e30be38b388798f3e6a I'm currently away but will work on the second patch asap.
Created attachment 242809 [details] [review] get_memory_total for OpenBSD Hi Martyn. Here's the second patch which implements get_memory_total() on OpenBSD.
Thanks Martyn, second patch is in a5dcf74772ad7800100905d93a11debf67644a38 I'll work on the last chunk this week, I still have to figure out how to include it in a clean way.
Ok, looking a bit more into this, I'm not sure which way to go. If I need to separate the OpenBSD parts, it will need some major re-factoring for the linux case as well. Martyn, any input on which angle I should start? Thanks.
Are you able to give more details about why you think we need to refactor the linux part too?
(In reply to comment #10) > Are you able to give more details about why you think we need to refactor the > linux part too? The problem is several parts of the code relies on /proc (get_uid_for_pid...), so it's not just tracker_control_general_run(). What I meant is that if I want to add OpenBSD support I will have to split several functions. I already have a get_pids() for OpenBSD but get_uid_for_pid() will require some changes as it takes a file path as argument (/proc/...) which we cannot do on OpenBSD.
(In reply to comment #11) > (In reply to comment #10) > > Are you able to give more details about why you think we need to refactor the > > linux part too? > > The problem is several parts of the code relies on /proc (get_uid_for_pid...), > so it's not just tracker_control_general_run(). > What I meant is that if I want to add OpenBSD support I will have to split > several functions. > I already have a get_pids() for OpenBSD but get_uid_for_pid() will require some > changes as it takes a file path as argument (/proc/...) which we cannot do on > OpenBSD. I am happy to split this stuff into proper functions in the libtracker-common/tracker-os-dependent*.[ch]. That probably makes sense if we use /proc in a bunch of places. My idea would be to just have the necessary APIs we need in tracker-control and other places in the code base like the get_uid_for_pid() and get_pids() and simply move them to the os-dependent area with #ifdefs for BSD. Is this what you considered as a "refactor" ? If so, that's fine by me. Over time we've increasingly worried less about non-Linux variants. At one point we supported Windows even so ... :)
> other places in the code base like the get_uid_for_pid() and get_pids() and > simply move them to the os-dependent area with #ifdefs for BSD. Is this what > you considered as a "refactor" ? If so, that's fine by me. Over time we've Yup that is what I meant indeed. Alright then, as soon as I can find some time I'll get on with it. Thanks Martyn.
Ok so, I reworked this completely. tracker_process_get_uid_for_pid() is now fully supported on OpenBSD and I tried to keep the diff as less intrusive as possible. As you can see in the diff, the feature is self-contained and does not need the other tracker_process_get_uid_for_pid()... * before: $ tracker daemon -p Could not open /proc: Error opening directory '/proc': No such file or directory Could not stat() file '/proc/26102/cmdline', Error when getting information for file '/proc/26102/cmdline': No such file or directoryFound 0 PIDs… * after: $ tracker daemon -p Found 5 PIDs... Found process ID 9147 for 'tracker-miner-apps' Found process ID 17853 for 'tracker-extract' Found process ID 1797 for 'tracker-miner-fs' Found process ID 13932 for 'tracker-miner-user-guides' Found process ID 9337 for 'tracker-store' * from another user who is not running any tracker process: $ tracker daemon -p Found 0 PIDs... I'd really appreciate comments or permission to push at some point since this is the last diff needed to make tracker runs on OpenBSD out-of-the-box. Thanks :-)
(In reply to Antoine Jacoutot from comment #14) > Ok so, I reworked this completely. > tracker_process_get_uid_for_pid() is now fully supported on OpenBSD and I > tried to keep the diff as less intrusive as possible. As you can see in the > diff, the feature is self-contained and does not need the other > tracker_process_get_uid_for_pid()... > > * before: > $ tracker daemon -p > Could not open /proc: Error opening directory '/proc': No such file or > directory > Could not stat() file '/proc/26102/cmdline', Error when getting information > for file '/proc/26102/cmdline': No such file or directoryFound 0 PIDs… > > * after: > $ tracker daemon -p > Found 5 PIDs... > Found process ID 9147 for 'tracker-miner-apps' > Found process ID 17853 for 'tracker-extract' > Found process ID 1797 for 'tracker-miner-fs' > Found process ID 13932 for 'tracker-miner-user-guides' > Found process ID 9337 for 'tracker-store' > > * from another user who is not running any tracker process: > $ tracker daemon -p > Found 0 PIDs... > > > I'd really appreciate comments or permission to push at some point since > this is the last diff needed to make tracker runs on OpenBSD out-of-the-box. > Thanks :-) Hi Antoine, I am happy for the patch to be applied. One thing though, the code has been moved directly into src/tracker/ now so the patch left as "reviewed" in this bug currently would need re-working. Feel free to rework the patch and apply it without further comment. I don't think the code changed, it just moved to the one place that was using it. Thanks :)
> Hi Antoine, I am happy for the patch to be applied. One thing though, the > code has been moved directly into src/tracker/ now so the patch left as > "reviewed" in this bug currently would need re-working. > > Feel free to rework the patch and apply it without further comment. > I don't think the code changed, it just moved to the one place that was > using it. Oh but... lol. I forgot to actually include my new patch :-) It has been reworked exactly the way you asked for, I just forgot to attach it...
Created attachment 300534 [details] [review] implement tracker_process_get_uid_for_pid()
Review of attachment 300534 [details] [review]: Looks good to me, thanks for the patch update :)
Awesome, it's in :-) 95ddae1b224c75b37b2dbe5f1f6b4cb69ea79bdd I'd be nice if you could cherry-pick it in the stable branch but if not, I can live with a local patch for the time being. Thanks Martyn!