[Bug 28915] MC does not save "old-ssl" flag

[bugzilla-daemon at freedesktop.org]

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

--- Comment #12 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-09-14 03:26:15 PDT ---
Trivial
=======

> +/**
> + * mcd_storage_get_plugin:
> ...
> +void
> +mcd_storage_delete_account (McdStorage *storage, const gchar *account)

Mismatched.

Serious
=======

> +static GValue *
> +_storage_dup_value (McdStorage *storage,
...
> +  if (value != NULL)
> +    g_clear_error (error);

This results in any non-integer, or missing integer, silently being interpreted
as 0: in particular, the integer accessors can only distinguish between
0-but-no-error and error by having a GError argument initially pointing to
NULL, and testing whether it has been set.

You want to have a temporary GError * variable (keyfile_error or something),
use that as the argument to g_key_file_get_thing(), discard the GValue if
keyfile_error is not NULL, and propagate keyfile_error into error.

You still mishandle string escapes here: the object path and string cases both
use g_key_file_get_value when they should use g_key_file_get_string. (The
difference is that if the file contains "a\tb", get_value returns 'a backslash
tb', whereas get_string returns 'a TAB b').

The double and boolean cases have no error handling, not even for "not
present".

The object path case can pile up GErrors (if g_key_file_get_value fails, then
its return is not a valid object path, and g_set_error is called again on a
non-empty GError). That's an assertion failure. It's also not valid to call
tp_dbus_check_valid_object_path on NULL. You narrowly avoided this failure mode
for integer range-checking, because 0 happens to be in range.

The g_warning case returns NULL without setting @error.

> +static gboolean
> +_storage_set_value (McdStorage *storage,

When writing a string into the file, you use g_key_file_set_value. You should
use g_key_file_set_string, which escapes special characters (test with "a\tb"
for instance).

Last time we mixed up the value and string versions, there were major
regressions involving people's disks filling up with backslashes. Let's not go
there again immediately before a stable branch.

> wrt: Why are you NIHing g_key_file_set_boolean et al?
> Largely so there's an identical path through the code for all types that 
> lets us compare the old value with the new value so we can signal back to 
> the caller whether anything actually changed (at which point we have the 
> raw string we want to stuff into the store anyway, so we might as well use
> it)

I think better logic for this would be:

* dup the old raw value
* set the new value using a more appropriate API:
  in other words, the code from keyfile_[gs]et_value, which we have
  already debugged!
* dup the new raw value
* if they differ, signal that it changed and write it into backends

> -    AccountDeleteData *delete_data;

It looks as though this struct is no longer used? If so, please delete.

> The McdStorage layer uses get/set _value, the actual plugins need to use _string

No, I think this commit should be reverted. The correct escaping to use depends
on the data type (string lists and strings are not the same escaping - string
lists additionally escape ";" within items as "\;"), and interpreting a string
list as a string or vice versa causes non-reversible information loss.

One day, we should probably change the on-disc format to not be GKeyFile. When
we're trying to make a stable branch yesterday is not that day.

>  match_account_parameter (McdAccount *account, const gchar *name,

You should check that value and conf are of compatible types. If you use
g_value_get_uint (conf) when conf contains anything except a uint (even an
int!), that's considered an error.

Not blockers, at this stage
===========================

Can any more of the McdStorage API become internal? I don't see why an Mcd
embedder should ever be calling mcd_storage_load()?

> @@ -1683,68 +1620,33 @@ mcd_account_manager_write_conf_async (McdAccountManager *account_manager,
...
> +        groups = mcd_storage_dup_accounts (storage, &n_accounts);

groups doesn't seem to be used in this method any more: you call this method
just to get n_accounts, emit a debug message with n_accounts in, then free
groups. I'd drop it and just say "all accounts".

Notes for the future
====================

"Move account load + initialisation into plugin-account" (also, at least, the
one for deletion) adds a static function but doesn't add it to the vtable. It
somewhat defeats the object of having small, digestible commits if the tests
wouldn't have passed after each one: reviewers still have to hold extra state
in their heads ("these changes will be needed to make it valid again") and
bisecting won't work.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.