[Bug 27727] account storage plugins can contain partial accounts

[bugzilla-daemon at freedesktop.org]

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

--- Comment #40 from Simon McVittie <simon.mcvittie at collabora.co.uk> ---
(In reply to comment #38)
> ::: mission-control-plugins/account-storage.c
> @@ +59,1 @@
> >   *   iface->delete = foo_plugin_delete;
> 
> Needs to be updated for delete_async/finish

Yeah.

> I don't think we want a default for list()
...
> I'm not sure if we want a default for get_attribute/parameter. Does it make
> sense for any storage to have no attribute and/or no param?

Already noted

> @@ -603,4 @@
> >  {
> >    McpAccountStorageIface *iface = MCP_ACCOUNT_STORAGE_GET_IFACE (storage);
> >  
> > -  SDEBUG (storage, "");
> 
> why removing this debug? All other methods still have it.

Your "simplify a bit plugin API" added it, so it was lost in the revert. I'll
put it back.

> 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.

Yeah, makes sense. Or I was wondering whether to change the signature of most
McdStorage methods so they take the plugin as an argument, now that McdAccount
always knows its own plugin; then we could get rid of McdStorage->accounts
altogether.

> > +      GVariant *old;
> > +
> > +      sa = ensure_stored_account (amd, account);
> 
> ensure_stored_account() should be called only from _create().

Makes sense. I'll see whether the tests still pass...

> > +          if (!tp_strdiff (escaped, new))
> > +            {
> > +              g_free (new);
> > +              return MCP_ACCOUNT_STORAGE_SET_RESULT_UNCHANGED;
> 
> Can't we move the untyped param to sa->parameters now, and return
> MCP_ACCOUNT_STORAGE_SET_RESULT_CHANGED?

One of the regression tests asserts that this won't be considered to be a write
- but you're probably right, we should consider changing that test.

> >    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)

sa->absent means the account is an empty file in a "higher-priority" directory,
created in order to "mask" an account in a "lower-priority" directory (because
we can't/shouldn't write to lower-priority directories - see my answer to your
query about g_get_system_data_dirs() below).

Yes, now that McdStorage tracks plugin "ownership" better, this sort of thing
can turn into a g_return*if_fail. I'd prefer to append that to the branch
rather than rebasing it into the middle, because we only got that guarantee a
couple of commits from the end.

> @@ +290,2 @@
> >    if (sa == NULL || sa->absent)
> >      return FALSE;
> 
> s/FALSE/NULL/

Good point.

> @@ +366,5 @@
> >    if (sa == NULL || sa->absent)
> >      {
> >        /* 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. I would add a g_warning() but still return
> success for the operation because there is no reason to fail the dbus call
> initiated from client side.

This does happen in the regression tests, now that delete_async is expected to
emit ::deleted (which it naturally ought to, I think - I don't like signals
that are emitted or not depending on the reason for a change). ::deleted ends
up calling back into delete_async.

We might be able to get rid of that by obeying the "already in storage" flag
better?

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

I hope not, but that's not actually all that g_get_system_data_dirs() returns:
it also returns anything the user has put in $XDG_DATA_DIRS. So for instance
you could have:

    XDG_DATA_HOME=/home/me/.local/share
    XDG_DATA_DIRS=/nfs/home/me/.local/share:/usr/local/share:/usr/share

I realize that doesn't necessarily make a huge amount of sense for accounts
either, but I think a lot of the value of the XDG base directories is that you
can rely on them being the same everywhere, so I'd prefer to do this Right™.

> > +  /* 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: ?

No, I don't think it should. If we fail to unlink the file representing the
account, and thus fail the async operation, I think we should continue to
believe that the account exists. The McdAccount will continue to exist, too,
and I don't think we want those getting out of sync.

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

Good catch, will fix.

> @@ +1233,5 @@
> >      DEBUG ("Account %s migrated, removing it",
> >             mcd_account_get_unique_name (ctx->account));
> >  
> > +    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

As noted above: good guess, but no.

> ::: src/mcd-storage.c
> @@ +505,1 @@
> >    while (store != NULL)
> 
> Now that we have a normal loop, I think it would look even better with:
> for (store = stores; store != NULL; store = store->next)

Yes.

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

OK, if you say so.

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

I didn't want to re-indent too much, to avoid making review even harder than it
was already going to be. I'll fix the coding style next.

> > +  if (mcp_account_storage_delete_finish (MCP_ACCOUNT_STORAGE (source),
> > +        res, &error))
> > +    {
> > +      DEBUG ("deleted account %s", (const gchar *) user_data);
> 
> I would add a gchar *account_name = user_data; declaration.

Reasonable.

> @@ +1956,5 @@
> > +    const gchar *escaped,
> > +    const GVariantType *type,
> > +    GError **error)
> > +{
> > +  return mcd_keyfile_unescape_variant (escaped, type, error);
> 
> That function can be inlined here, it's the only user.

It is? OK, fine. I think MC used it internally in some intermediate version of
this branch.

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