[Bug 24257] TpAccount, TpAccountManager: prepare never terminates if the object is invalidated

[bugzilla-daemon at freedesktop.org]

http://bugs.freedesktop.org/show_bug.cgi?id=24257





--- Comment #10 from David Laban <david.laban at collabora.co.uk>  2009-11-10 18:21:22 PST ---
(In reply to comment #9)
> <http://git.collabora.co.uk/?p=user/alsuren/telepathy-glib.git;a=commitdiff;h=aa085b71d2ab0d91e347484c578a200dae581a2c>:
> 
> newline between statement and "if" block in the second hunk, please
> 
Ah. I wanted to group the error with the if statement, since that's the only
place it's used. I defer to your style though.

> I'd prefer a one-liner like "fd.o#24257: tp_account_prepare: fail pending calls
> if invalidated"; likewise for the AM.
> 
"fd.o#24257: tp_account_prepare: fail if invalidated" is clearer when truncated
to 50 chars. "fd.o#24257: tp_account_manager_prepare: fail if invalidated" is
harder, but easy enough to infer based on the previous commit message.

> <http://git.collabora.co.uk/?p=user/alsuren/telepathy-glib.git;a=commitdiff;h=d9a7adfca0941e0c6b468e8f793b7380f22d9f21>:
> 
> The check should be a g_return_val_if_fail, and should probably use
> tp_dbus_check_valid_object_path() directly. 
I'm slightly confused by the difference between return.*if_fail and assert.
They are both used for programmer errors, and both cause the program to abort
when I use them. I suppose they do different things if you turn asserts off or
something?
> It can replace the existing path !=
> NULL check, since tp_dbus_check_valid_object_path() will itself
> g_return_val_if_fail on NULL.
tp_account_new() calls tp_account_parse_object_path() which calls
tp_dbus_check_valid_object_path() among other things. 

Not sure whether it's worth repeating lots of complicated asserts, but != NULL
is cheap. I just wanted to fail as soon after the error occurred as possible. I
might be doing premature optimisation though. 
Changed to g_return_val_if_fail (account != NULL, NULL); and changed the commit
message to reflect this. Tell me if you want 
g_return_val_if_fail (tp_account_parse_object_path (object_path, NULL, NULL,
NULL, error), NULL); as well.
> <http://git.collabora.co.uk/?p=user/alsuren/telepathy-glib.git;a=commitdiff;h=90837609f00c62ade6fb980dcdcb5a1b81f8bb3f>:
> 


> If it's unused, I'd prefer "int dummy;", which appears in some of the examples
> too.
Thanks. I wasn't sure the best way to make this obvious. That's why I commented
it so heavily.
> 
> >+simple_account_manager_get_property (GObject *object,
> 
> If you're responding to my coding style nitpicking anyway, I'd like to request
> a blank line between cases (so between break and default, here) :-)
> 

> 
> <http://git.collabora.co.uk/?p=user/alsuren/telepathy-glib.git;a=commitdiff;h=1744d8be9481a6e09eb583906e4e419f464056bd>:
> +  // TODO: call things like tp_account_manager_create_account_async.
> +  // This clearly needs to be done in the prepare callback. Might have to
> +  // think of a way to refactor this.
> 
> Either fix this, or file a trivial/low bug for it (and use C-style comments
> rather than C++-style).
> 
My bad. Forgot to remove the comment. I guess the part about testing the rest
of AccountManager still stands though. Will file a bug in the morning.

> <http://git.collabora.co.uk/?p=user/alsuren/telepathy-glib.git;a=commitdiff;h=8ab93a727864f0117c08b789cc62a438e4adf15f>:
> > +  g_assert (test->prepared == tp_account_manager_is_prepared (test->am, TP_ACCOUNT_MANAGER_FEATURE_CORE));
> 
> g_assert_intcmp (t_a_m_i_p (), ==, t->p) ?
> 
Fair point. Actually, should I be putting !! in front of bools before comparing
them, or is gboolean guaranteed to be 1 or 0 somehow?

> > +assert_failed_action (gpointer script_data,
> 
> Don't you want to free the GError here? If tests don't leak, it's easier to
> valgrind the library. (Either put a const GError * in the struct, or copy any
> unowned errors.)
> 
Ah memory management. I'll get used to it eventually. I never did run refdebug
or valgrind on anything, did I? Oh well. Maybe tomorrow.

http://git.collabora.co.uk/?p=user/alsuren/telepathy-glib.git;a=shortlog;h=refs/heads/fix-24257-alsuren
is now the product of my 2:00am brain.


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