GNOME Bugzilla – Bug 728280
platform_get_argv0: fix sysctl(3) use on OpenBSD
Last modified: 2014-04-20 21:17:53 UTC
Created attachment 274386 [details] [review] fix sysctl(3) use on OpenBSD Hi. This patch rework platform_get_argv0 to properly use sysctl(3) on OpenBSD and add some sanity checks. Regression tests are happy: ./option-argv0 --verbose --debug-log GTest: random seed: R02Sb5d798ac22b0eceee4e4242d53855b53 {*LOG(binary):LOG*} {*LOG(start suite):{}:LOG*} {*LOG(start suite):{option}:LOG*} {*LOG(start):{/option/argv0}:LOG*} GTest: run: /option/argv0 GTest: result: OK {*LOG(stop):{/option/argv0}:(0;0;0.000962):LOG*} {*LOG(stop suite):{option}:LOG*} {*LOG(stop suite):{}:LOG*}
Review of attachment 274386 [details] [review]: ::: glib/goption.c @@ +1770,3 @@ int mib[] = { CTL_KERN, KERN_PROC_ARGS, getpid(), KERN_PROC_ARGV }; + args = NULL; + len = 128; What is the purpose of initialising this? Is it an in/out for the next call? If so, 128 seems a bit small... @@ +1777,1 @@ + args = g_malloc0 (len + 1); I'm fairly sure this must be incorrect somehow. What is 'len' as returned by the syscall? args is a 'char **'. Is this the number of 'char *' in the array or is it the byte length of the array? Probably makes more sense to use g_new() here... or if 'len' really is the byte length then you need to add sizeof(char*) for the NULL -- not 1. If 'len' is a bytesize rather then a length, the name 'size' would be nicer. @@ +1777,2 @@ + args = g_malloc0 (len + 1); + if (args == NULL) drop this check please -- g_malloc() never fails. @@ +1783,3 @@ + + buf[0] = '\0'; + for (argv = args; *argv != NULL; argv++) { This is much easier written g_strjoinv(" ", ...); But if the goal is to extract only the basename of argv0 then why do you paste together all of the arguments as well? @@ +1790,3 @@ + + g_free (args); + cmdline = g_strdup_printf ("%s", buf); better written g_strdup(buf); ... but there is little point in allocating this string temporarily in order to call basename() on it and free it just below. You could just call basename() on buf directly.
Created attachment 274417 [details] [review] clean up platform_get_argv0 (OpenBSD) Hi Ryan. Sorry I was reviewing a lot of similar code and I totally missed the point for that part... my copy/paste was too savage ;-) Anyway, since I am around, this small patch cleans up unneeded headers and use g_realloc instead of realloc.
Created attachment 274427 [details] [review] clean up platform_get_argv0 (OpenBSD) v2
Hi Ryan. Any other input on that last diff? Thanks :-)
Nope. Looks like an improvement -- please go ahead.
Thanks Ryan. Pushed as 9352cdb.