|
Description
Miloslav Trmac
2013-09-18 17:24:00 UTC
Created attachment 86079 [details] [review] 0004-sessionmonitor-systemd-cleanup-Deduplicate-code-path.patch Ok, I came back to this, and fixed up those two above patches, and I have two more. Created attachment 88855 [details] [review] 0001-PolkitSystemBusName-Add-public-API-to-retrieve-Unix-.patch Created attachment 88856 [details] [review] 0002-sessionmonitor-systemd-Deduplicate-code-paths.patch Created attachment 88857 [details] [review] 0003-examples-cancel-Fix-to-securely-lookup-subject.patch Created attachment 88858 [details] [review] 0004-Use-G_GNUC_BEGIN_IGNORE_DEPRECATIONS-to-avoid-warnin.patch Comment on attachment 88855 [details] [review] 0001-PolkitSystemBusName-Add-public-API-to-retrieve-Unix-.patch Review of attachment 88855 [details] [review]: ----------------------------------------------------------------- Looks good. Comment on attachment 88857 [details] [review] 0003-examples-cancel-Fix-to-securely-lookup-subject.patch Review of attachment 88857 [details] [review]: ----------------------------------------------------------------- OK. Comment on attachment 88858 [details] [review] 0004-Use-G_GNUC_BEGIN_IGNORE_DEPRECATIONS-to-avoid-warnin.patch Review of attachment 88858 [details] [review]: ----------------------------------------------------------------- ::: src/polkit/polkitpermission.c @@ +125,2 @@ > if (permission->subject == NULL) > permission->subject = polkit_unix_process_new (getpid ()); In this case we know precisely the attributes of the process; wouldn't it be cleaner to call polkit_unix_process_new_for_owner (getpid(), 0 /* look up, which is safe */, getuid()); ? That would amount to the same thing, be marginally faster because it avoids _polkit_unix_process_get_owner(), and most importantly not suggest that the code is somehow problematic. ::: src/polkit/polkitsystembusname.c @@ +393,2 @@ > ret = polkit_unix_process_new (pid); > + G_GNUC_END_IGNORE_DEPRECATIONS We actually can get the correct UID from D-Bus here; wouldn't it make sense to provide the correct information? (The startup time would remain unreliable.) OTOH at least the in-tree users only use the PID, so this would be adding one more lookup without any tangible benefit. I'm not sure. ::: src/programs/pkcheck.c @@ +397,3 @@ > if (sscanf (argv[n], "%i,%" G_GUINT64_FORMAT ",%u", &pid, &pid_start_time, &uid) == 3) > { > subject = polkit_unix_process_new_for_owner (pid, pid_start_time, uid); Being a pedant, I'd prefer the IGNORE_DEPRECATIONS marker only around the 2 polkit_unix_process_* calls instead of also including the sscanfs, and especially the one non-deprecated call. It would also be consistent with the polkitsubject hunk above. ::: test/polkitbackend/test-polkitbackendjsauthority.c @@ +78,3 @@ > caller = polkit_unix_process_new (getpid ()); > subject = polkit_unix_process_new (getpid ()); > + G_GNUC_END_IGNORE_DEPRECATIONS Here I'd also prefer calling the non-deprecated API instead. @@ +346,3 @@ > caller = polkit_unix_process_new (getpid ()); > subject = polkit_unix_process_new (getpid ()); > + G_GNUC_END_IGNORE_DEPRECATIONS Here I'd also prefer calling the non-deprecated API instead. Comment on attachment 88856 [details] [review] 0002-sessionmonitor-systemd-Deduplicate-code-paths.patch Review of attachment 88856 [details] [review]: ----------------------------------------------------------------- Yes, the pid->session duplication removal is a good thing. OTOH see comment on patch #4 - it might make sense polkit_system_bus_name_get_process_sync() to look up both PID and UID (I really don't know), and in that case the UID lookup is wasteful. Perhaps we might end up with an internal-only polkit_system_bus_name_get_unix_pid() that avoids the UID lookup? Or then again, it might not be worth worrying about. A similar cleanup can be done in sessionmonitor.c, but I don't have an easy way to test it, so it's probably best left alone. (In reply to comment #10) > > OTOH see comment on patch #4 - it might make sense > polkit_system_bus_name_get_process_sync() to look up both PID and UID (I > really don't know), and in that case the UID lookup is wasteful. That's a very good point; I think we should do this even if it's not actually used internally, because it's clearly the correct thing to do, and who knows - maybe a later refactoring will make it important. Or maybe it'll be useful for external users. I think it'll be fine in terms of efficiency if we make two async calls, then block on them both returning. https://bugs.freedesktop.org/show_bug.cgi?id=54445 is an in-progress bug to make exactly this more efficient. If it's OK I'd like to do it as a separate patch on top, as it more clearly separates pure code deduplication from an improvement. Comment on attachment 88855 [details] [review] 0001-PolkitSystemBusName-Add-public-API-to-retrieve-Unix-.patch 0001 committed. Comment on attachment 88857 [details] [review] 0003-examples-cancel-Fix-to-securely-lookup-subject.patch 0003 committed. Created attachment 88934 [details] [review] 0001-PolkitSystemBusName-Retrieve-both-pid-and-uid.patch Additional patch to follow 0002, tested and works on top of gnome-continuous buildmaster. Created attachment 88944 [details] [review] 0001-Port-internals-non-deprecated-PolkitProcess-API-wher.patch Created attachment 88945 [details] [review] 0002-Use-G_GNUC_BEGIN_IGNORE_DEPRECATIONS-to-avoid-warnin.patch (In reply to comment #9) > In this case we know precisely the attributes of the process; wouldn't it be > cleaner to call > > polkit_unix_process_new_for_owner (getpid(), 0 /* look up, which is safe */, > getuid()); Indeed, I should have looked more carefully for calls which could be updated. I split this into two patches - the updated calls, and the ones we must ignore. Thanks for the careful review! Created attachment 89028 [details] [review] 0001-Port-internals-non-deprecated-PolkitProcess-API-wher.patch This one compiles, forgot to amend this commit before filing. Comment on attachment 88934 [details] [review] 0001-PolkitSystemBusName-Retrieve-both-pid-and-uid.patch Review of attachment 88934 [details] [review]: ----------------------------------------------------------------- ::: src/polkit/polkitsystembusname.c @@ +380,5 @@ > + g_assert (!data->retrieved_pid); > + data->retrieved_pid = TRUE; > + data->pid = value; > + } > + } Is it actually documented that GDbus will call the callbacks in the same order as the requests have been issued? I agree that it makes sense given the underlying implementation, still... Perhaps I just worry too much. @@ +406,5 @@ > + g_main_context_push_thread_default (tmp_context); > + > + /* Do two async calls as it's basically as fast as one sync call. > + */ > + g_dbus_connection_call (connection, Adds trailing whitespace @@ +418,5 @@ > + -1, > + cancellable, > + on_retrieved_unix_uid_pid, > + &data); > + g_dbus_connection_call (connection, Adds trailing whitespace @@ +467,4 @@ > GCancellable *cancellable, > GError **error) > { > + PolkitSubject *ret = NULL; "connection" can be removed entirely. @@ +504,5 @@ > GCancellable *cancellable, > GError **error) > { > + PolkitUnixUser *ret = NULL; > + GDBusConnection *connection = NULL; "connection" can be removed entirely. @@ +512,5 @@ > g_return_val_if_fail (cancellable == NULL || G_IS_CANCELLABLE (cancellable), NULL); > g_return_val_if_fail (error == NULL || *error == NULL, NULL); > > + if (!polkit_system_bus_name_get_creds_sync (system_bus_name, &uid, NULL, > + cancellable, error)) (There's no direct benefit to changin this, but this patch makes the code easier to adapt for bug 54445 (or to add caching), which makes sense as well. So I'm fine with this, just highlighting.) Comment on attachment 88945 [details] [review] 0002-Use-G_GNUC_BEGIN_IGNORE_DEPRECATIONS-to-avoid-warnin.patch Review of attachment 88945 [details] [review]: ----------------------------------------------------------------- ACK, and thanks for the update. Comment on attachment 89028 [details] [review] 0001-Port-internals-non-deprecated-PolkitProcess-API-wher.patch Review of attachment 89028 [details] [review]: ----------------------------------------------------------------- ACK, and thanks for the update. (In reply to comment #19) > Is it actually documented that GDbus will call the callbacks in the same > order as the requests have been issued? I agree that it makes sense given > the underlying implementation, still... Perhaps I just worry too much. Yep, I can't offhand find this as part of the spec, but see: http://lists.freedesktop.org/archives/dbus/2009-November/011927.html Fixed up for all of the other comments, thanks! |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.