[Bug 27727] account storage plugins can contain partial accounts

[bugzilla-daemon at freedesktop.org]

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

--- Comment #38 from Xavier Claessens <xclaesse at gmail.com> ---
Comment on attachment 89237
  --> https://bugs.freedesktop.org/attachment.cgi?id=89237
diff between master-without-all-revert and Simon's branch

Review of attachment 89237:
 --> (https://bugs.freedesktop.org/page.cgi?id=splinter.html&bug=27727&attachment=89237)
-----------------------------------------------------------------

Here is my full review, it repeat what I did in patch-by-patch, but add a few
stuff I think.

::: mission-control-plugins/account-storage.c
@@ +59,1 @@
>   *   iface->delete = foo_plugin_delete;

Needs to be updated for delete_async/finish

@@ +157,2 @@
>  {
> +  return NULL;

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

@@ +207,5 @@
> +    const GVariantType *type,
> +    McpParameterFlags *flags)
> +{
> +  return NULL;
> +}

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?

@@ +470,3 @@
>   *
> + * There is a default implementation, which just returns %NULL.
> + * All account storage plugins must override this method.

If all plugins must override, better not have a default then, so we get a warn
in g_return_val_if_fail (iface->get_attribute != NULL, FALSE); below.

@@ +508,5 @@
> + *
> + * Retrieve a parameter.
> + *
> + * There is a default implementation, which just returns %NULL.
> + * All account storage plugins must override this method.

likewise.

@@ -603,4 @@
>  {
>    McpAccountStorageIface *iface = MCP_ACCOUNT_STORAGE_GET_IFACE (storage);
>  
> -  SDEBUG (storage, "");

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

@@ +739,3 @@
>   */
>  gboolean
> +mcp_account_storage_delete_finish (McpAccountStorage *storage,

Doc should tell about default implementation? Probably saying that it should
not reply on it if _async() has been overridden. Or tell it's to not override
if _async() uses a GTask?

::: src/mcd-account-manager-default.c
@@ +186,5 @@
> +
> +      sa = lookup_stored_account (amd, account);
> +
> +      if (sa == NULL)
> +        return MCP_ACCOUNT_STORAGE_SET_RESULT_UNCHANGED;

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.

@@ +200,5 @@
> +  else
> +    {
> +      GVariant *old;
> +
> +      sa = ensure_stored_account (amd, account);

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

I would do:

sa = lookup_stored_account (amd, account);
g_return_val_if_fail (sa != NULL, MCP_ACCOUNT_STORAGE_SET_RESULT_FAILED);

if (val == NULL)
  {
    ...
  }
else
  {
    ...
  }

@@ +214,5 @@
> +
> +          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?

@@ +251,3 @@
>  
> +      if (sa == NULL)
> +        return MCP_ACCOUNT_STORAGE_SET_RESULT_UNCHANGED;

same as above

@@ +258,5 @@
>    else
>      {
> +      GVariant *old;
> +
> +      sa = ensure_stored_account (amd, account);

same as above

@@ +290,1 @@
>    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)

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

s/FALSE/NULL/

@@ +313,3 @@
>  
> +  if (sa == NULL || sa->absent)
> +    return FALSE;

same as above

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

@@ +392,2 @@
>  
> +  for (iter = g_get_system_data_dirs ();

we have distro-installed accounts?

@@ +422,5 @@
>      }
>  
> +  /* 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: ?

::: src/mcd-account-manager.c
@@ +294,5 @@
> +    }
> +    else
> +    {
> +        WARNING ("%s", error->message);
> +        goto finish;

error is leaked

@@ +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

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

@@ +547,2 @@
>  {
> +  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).

@@ +1274,4 @@
>      }
>    else
>      {
> +      g_assert_not_reached ();

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

@@ +1782,5 @@
> +
> +  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.

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

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