Code review comment for lp://staging/~ted/unity8-policy-kit/initial-work

On Thu, 2016-04-14 at 18:10 +0000, Charles Kerr wrote:
> auth-manager.h:
> member fields 'inFlight' and 'thread' don't use the same 'member
> field' notation as other classes
Fixed r35
> agent-glib.cpp's initiate_authentication:
> do we know for certain that all the arguments passed into this will be non-null?
> otherwise we'll get a crash when they're converted into std::string as we pass them along as args to agentlib->cpp->authRequest()
>

They *should* be, but I think protecting isn't a bad thing. Helper
inline func in r36
> auth-manager.cpp
> * I'm a little concerned that notify_init() and get_server_caps() are called basically immediately on startup,
> and that startup happens on unity8 startup.
> indicator-datetime did something similar and had a startup timing issue with unity-notifications,
> it now watches for the notifications name to be owned on the bus
>

I think we've fixed that in unity8's startup now, it should export the
interface and block startup until it is complete. So things that are
started unity8 should work. Since we're exiting and will get restarted
on that case, I'm okay leaving it as-is now, and then watching daisy
for reports.
> auth-manager.cpp
>
> >
> > if (std::string(capname) == "x-canonical-private-synchronous")
> >

>
> I'm still paranoid about passing untested input into std::string(), IMO !g_strcmp0(capname, "x-canonical-private-synchronous") is safer.
> Also, can break out of the loop once hasDialogs is true
>

One of my favorite features of C++ is being about use == with strings,
so I put an if nullptr continue statement in instead of gstrcmp :-)
 r37
> auth-manager.cpp
>
> >
> > auto success = thread.executeOnThread<bool>([]() {
> > thread.executeOnThread<bool>([this]() {
> >

>
> same question as earlier, these returned bools are unused, so why not executeOnThread<>(... instead?
>

Same answer ;-)
> auth-manager.cpp
>
> >
> > auto capname = reinterpret_cast<const gchar *>(cap->data);
> >

>
> static_cast because cap->data is a void*
>

Fixed r38
> auth-manger.cpp
>
> >
> > return thread.executeOnThread<std::string>([this, &action_id, &message, &icon_name, &cookie, &identities, &finishedCallback]() {
> >

>
> after a bunch of nitpicking, here's a legit Needs Fixing, none of these captured variables should be references because they'll be
> pointing at out-of-scope objects as soon as agent_glib_class_init()'s initiate_dead objects lambda exits
>

No, because the executeOnThread is blocking so the lambda will finish
executing before the execute returns. The buildAuthentication lambda
does make copies because it is executed later.
> auth-manager.cpp
>
> >
> > throw std::runtime_error("Handle for Authentication '" + cookie +
> >

>
> where is this caught?
> this doesn't seem like it should be a fatal error but I don't see where it's caught?
>

I was thinking it should be fatal. Generally this would mean our data
structures are corrupt, let Upstart restart us.