[Bug 27727] account storage plugins can contain partial accounts

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=27727

--- Comment #44 from Simon McVittie <simon.mcvittie at collabora.co.uk> ---
Looks as though there are still a couple of things I missed.

(In reply to comment #32)
> migrate_delete_account_cb() looks weird, why don't you put directly
> mcd_account_delete_debug_cb as callback and free the MigrateCtx before the
> async call?

Not done, I explained why above.

> While we are adding async API, why omitting the GCancellable? I think you
> can just have one and put it on your GTask, it will do the right thing for
> you.

Done.

(In reply to comment #33)
> Not sure to understand why MC would ever create a system bus, modules doing
> it behind our back?

I explained why above.

> You can also just use g_assert_no_error() instead of
> more complex error handling, no?

Done.

(In reply to comment #34)
> mcp_account_storage_delete_finish()'s doc should tell about the default
> implementation.

I said in delete_async that if you override one, you need to override both. Is
that sufficient?

(In reply to comment #35)
> In mcd_storage_add_account_from_plugin() I would keep the comment my branch
> adds:
> /* This will fill our parameters/attributes tables */ because it's nowhere
> close to obvious.

The relevant code went away, so, not needed.

(In reply to comment #36)
> I would make it g_return_val_if_fail (sa != NULL,
> MCP_ACCOUNT_STORAGE_SET_RESULT_FAILED); because we are no longer setting on
> all backends.

Done.

> ensure_stored_account() should only be called from _create(), right? Same as
> above, I would g_return_val_if_fail().

Done; it's only called from create() and load() now.

> If we have an untyped param, now is a good time to move to to sa->parameters
> and consider that to be a change.

**** not done yet ****

> > +      if (sa == NULL)
> > +        return MCP_ACCOUNT_STORAGE_SET_RESULT_UNCHANGED;
> 
> same as above, g_return_val_if_fail()

Done

> > +      sa = ensure_stored_account (amd, account);
> 
> same as above: s/ensure/lookup/ and g_return_val_if_fail()

Done

(In reply to comment #38)
> >   *   iface->delete = foo_plugin_delete;
> 
> Needs to be updated for delete_async/finish

Done

> I don't think we want a default for list()

Fixed

> I'm not sure if we want a default for get_attribute/parameter

Deleted

> If all plugins must override, better not have a default then

Fixed

> > + * All account storage plugins must override this method.
> 
> likewise.

Fixed

> why removing this debug? All other methods still have it.

Added (and improved)

> Doc should tell about default implementation? Probably saying that it should
> not reply on it if _async() has been overridden.

Done

> I would g_return_val_if_fail (sa != NULL,
> MCP_ACCOUNT_STORAGE_SET_RESULT_FAILED); because MC now guarantees that it
> calls _set_parameter() on the right storage only.

Done

> ensure_stored_account() should be called only from _create().

Done (well, it's also called from load())

> sa = lookup_stored_account (amd, account);
> g_return_val_if_fail (sa != NULL, MCP_ACCOUNT_STORAGE_SET_RESULT_FAILED);
> 
> if (val == NULL)
>   {
>     ...
>   }
> else
>   {
>     ...
>   }

**** not yet done ****

> Can't we move the untyped param to sa->parameters now, and return
> MCP_ACCOUNT_STORAGE_SET_RESULT_CHANGED?

**** not yet done ****

> > +      if (sa == NULL)
> > +        return MCP_ACCOUNT_STORAGE_SET_RESULT_UNCHANGED;
> 
> same as above

Done

> > +      sa = ensure_stored_account (amd, account);
> 
> same as above

Done

> >    if (sa == NULL || sa->absent)
> 
> Not sure what is sa->absent, but sa==NULL should be a g_return_val_if_fail
> (sa != NULL, NULL)

Done

> >    if (sa == NULL || sa->absent)
> >      return FALSE;
> 
> s/FALSE/NULL/

I think I fixed all these, let me know if there are more

> >        /* Apparently we never had this account anyway. The plugin API
> >         * considers this to be "success". */
> > +      g_task_return_boolean (task, TRUE);
> 
> Not sure... that would be a sign that something is broken in MC's
> representation of accounts

Fixed!

> @@ +392,2 @@
> >  
> > +  for (iter = g_get_system_data_dirs ();
> 
> we have distro-installed accounts?

Explained above, no action taken

> > +  /* clean up the mess */
> > +  g_hash_table_remove (amd->accounts, account);
> > +  mcp_account_storage_emit_deleted (self, account);
> 
> I'm not sure, but shouldn't this be under the finally

Explained above why not

> > +        WARNING ("%s", error->message);
> > +        goto finish;
> 
> error is leaked

Fixed

> > +    mcd_account_delete_async (ctx->account, MCD_DBUS_PROP_SET_FLAG_NONE,
> > +                              migrate_delete_account_cb, ctx);
> 
> Free ctx from here and use mcd_account_delete_debug_cb

Explained why not

> Now that we have a normal loop, I think it would look even better with:
> for (store = stores; store != NULL; store = store->next)

Done

> tbh the ref here is just asking for leaks. Either deep-copy or return
> internal pointer with (transfer none).

Done, (transfer none)

> isn't it easier to have g_return_val_if_fail(plugin != NULL, FALSE); to
> remove a big if block?

Re-indented

> > +      DEBUG ("deleted account %s", (const gchar *) user_data);
> 
> I would add a gchar *account_name = user_data; declaration.

Done

> > +  return mcd_keyfile_unescape_variant (escaped, type, error);
> 
> That function can be inlined here, it's the only user.

Done

-- 
You are receiving this mail because:
You are the QA Contact for the bug.