Created attachment 93126 [details] [review] [tp-glib master 1/2] tp_connection_manager_param_dup_variant_type: add In order for this to work, TpProtocol now ignores parameters whose D-Bus signatures are not a syntactically valid single complete type. This is helpful for Mission Control to be able to migrate parameters from untyped to typed storage - it stores parameters in terms of GVariant. Created attachment 93127 [details] [review] [tp-glib master 2/2] TpProtocol: fix some suspicious memory management It works fine as long as a preallocated GArray doesn't move its memory buffer for no reason, but it's good to be obviously correct. Spotted while working on the previous commit. Created attachment 93128 [details] [review] [MC 1/9] Require account plugins to provide the ability to list parameters --- Unanswered question: should get_typed_parameters() also return the parameter flags, like get_parameter() does? The parameter flags used to be for the SECRET flag, but having abolished that flag, I'm not sure what else we'd use it for. Or maybe while we're breaking API already, we should delete the flags altogether? (Worst case, callers can use get_parameter(..., NULL, &flags) to get just the flags, although that's pretty horrible.) (In reply to comment #3) > [MC 1/9] Require account plugins to provide the ability to list parameters The commit message doesn't say, but this patch also adds a get_features() function, because we need that to be able to say "this backend can store types". I wonder whether to rename it to get_flags() so we can use it for things that aren't necessarily "positive", like MIGRATED_FROM_0_x? It's per-account in order to make it more versatile - the only flag that currently exists is per-backend, but while we can easily use per-account flags for per-backend features, the converse isn't true. Created attachment 93129 [details] [review] [MC 2/9] mcd_account_get_parameter: take a TpConnectionManagerParam We never call this function without a TpConnectionManagerParam readily available - which is just as well, because it looks as though it would crash if mcd_manager_get_protocol_param() failed. Created attachment 93130 [details] [review] [MC 3/9] mcd_account_dup_protocol: factor out Created attachment 93131 [details] [review] [MC 4/9] Squash mcd-account-connection.c into mcd-account.c There's hardly anything there any more anyway. Take the opportunity to replace _mcd_account_set_connection_context and _mcd_account_get_connection_context with simple access to the priv struct. --- This is just refactoring; it means we can avoid having to provide accessors or public functions for everything used by mcd-account-connection. Created attachment 93132 [details] [review] [MC 5/9] Simplify the account connection pipeline, and make it private --- More simple refactoring. Created attachment 93133 [details] [review] [MC 6/9] _mcd_account_dup_parameters: try to get parameters' types from backend Now that I've deleted ExternalAccountStorage support, we have two uses for this function: * get the parameters to be passed to RequestConnection * get the parameters for our own D-Bus API (PropertiesChanged, GetAll, etc.) For the former, we should know the types already, because we should already have a concrete CM/protocol in mind by the time we get here. For the latter, ideally we shouldn't need the CM's types at all: if the backend is storing parameters with types, it's arguably more correct for Parameters to contain what the user stored, even if that isn't an exact match for what the CM wants. --- This mostly fixes wjt's long-standing "FIXME: this is ridiculous" comment. > @@ -4975,10 +5045,19 @@ _mcd_account_connection_begin (McdAccount *account, ... >+ ctx->params = mcd_account_coerce_parameters (account, protocol); Xavier: just after this point is a good place for you to splice in the account path suffix for Bug #74030, or anything else that requires injecting extra parameters. In particular, you'll now have access to the TpProtocol to query whether it has the parameter. You'd have to either deep-copy the a{sv}, or change mcd_account_coerce_parameters() so it explicitly documents what memory-allocation model it uses for the a{sv}'s contents. Created attachment 93134 [details] [review] [MC 7/9] _mcd_account_reconnect: if the account isn't valid, just disconnect It is not valid to call _mcd_account_connection_begin() unless the account has a TpProtocol; in particular, if the account is "valid", then we know it does have a TpProtocol. --- Fixes the FIXME from the previous commit, but I can also backport this to 5.16 if people want - I expect it would apply cleanly, except for the patch-band that deletes the FIXME. Created attachment 93137 [details] [review] [MC 8/9] mcd_account_check_validity, mcd_account_check_parameters: be synchronous There isn't actually anything in these functions that needs to be async. --- ... and there was much rejoicing. Created attachment 93138 [details] [review] [MC 9/9] Opportunistically migrate accounts from untyped to typed Parameters --- This requires a dependency bump to the tp-glib version that has Attachment #93126 [details], which will hopefully be 0.23.1. Please imagine I'd included that; when this is all ready for merge, I'll merge the tp-glib bits, do a release, and merge the MC bits. Comment on attachment 93126 [details] [review] [tp-glib master 1/2] tp_connection_manager_param_dup_variant_type: add Review of attachment 93126 [details] [review]: ----------------------------------------------------------------- ::: telepathy-glib/connection-manager.c @@ +2605,5 @@ > + * @param: a parameter supported by a #TpConnectionManager > + * > + * <!-- --> > + * > + * Returns: the #GVariantType of the parameter shouldn't you annotate this (transfer full)? Comment on attachment 93127 [details] [review] [tp-glib master 2/2] TpProtocol: fix some suspicious memory management Review of attachment 93127 [details] [review]: ----------------------------------------------------------------- ++ (In reply to comment #3) > Created attachment 93128 [details] [review] [review] > [MC 1/9] Require account plugins to provide the ability to list parameters > > --- > > Unanswered question: should get_typed_parameters() also return the parameter > flags, like get_parameter() does? The parameter flags used to be for the > SECRET flag, but having abolished that flag, I'm not sure what else we'd use > it for. > > Or maybe while we're breaking API already, we should delete the flags > altogether? > > (Worst case, callers can use get_parameter(..., NULL, &flags) to get just > the flags, although that's pretty horrible.) Humm wouldn't it be easier to return a struct/object then? Something like TpConnectionManagerParam so we can have method to check if the param is secret or not. Comment on attachment 93129 [details] [review] [MC 2/9] mcd_account_get_parameter: take a TpConnectionManagerParam Review of attachment 93129 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 93130 [details] [review] [MC 3/9] mcd_account_dup_protocol: factor out Review of attachment 93130 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 93131 [details] [review] [MC 4/9] Squash mcd-account-connection.c into mcd-account.c Review of attachment 93131 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 93132 [details] [review] [MC 5/9] Simplify the account connection pipeline, and make it private Review of attachment 93132 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 93133 [details] [review] [MC 6/9] _mcd_account_dup_parameters: try to get parameters' types from backend Review of attachment 93133 [details] [review]: ----------------------------------------------------------------- ::: src/mcd-account.c @@ +3699,5 @@ > + } > + > + /* MC didn't always know parameters' types, so it might need the CM > + * (or .manager file) to be around to tell it whether "true" > + * is a string or a boolean⦠this is ridiculous, but backwards-compatible. typo. Comment on attachment 93134 [details] [review] [MC 7/9] _mcd_account_reconnect: if the account isn't valid, just disconnect Review of attachment 93134 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 93137 [details] [review] [MC 8/9] mcd_account_check_validity, mcd_account_check_parameters: be synchronous Review of attachment 93137 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 93138 [details] [review] [MC 9/9] Opportunistically migrate accounts from untyped to typed Parameters Review of attachment 93138 [details] [review]: ----------------------------------------------------------------- ++ (In reply to comment #13) > [tp-glib master 1/2] tp_connection_manager_param_dup_variant_type: add > shouldn't you annotate this (transfer full)? Er, yes. I assume the telepathy-glib bits are OK with that change? (In reply to comment #15) > Humm wouldn't it be easier to return a struct/object then? Something like > TpConnectionManagerParam so we can have method to check if the param is > secret or not. If we're going to allow for future expansion like this, I think I'd rather make the vfunc return a list of names (strv of strings, probably) like get_untyped_parameters() does, and iterate through that calling get_parameter() for each one - that seems simpler than inventing objects. Would that be OK from your point of view? If we ever need metadata other than flags, we can have a HAS_METADATA flag and an optional get_parameter_metadata(), or something. (In reply to comment #4) > The commit message doesn't say, but this patch also adds a get_features() > function, because we need that to be able to say "this backend can store > types". I wonder whether to rename it to get_flags() so we can use it for > things that aren't necessarily "positive", like MIGRATED_FROM_0_x? Having thought about this since last week, my conclusion is "yes, they should be called Flags". I'll cut it out into its own patch, too. (In reply to comment #20) > > + /* MC didn't always know parameters' types, so it might need the CM > > + * (or .manager file) to be around to tell it whether "true" > > + * is a string or a boolean⦠this is ridiculous, but backwards-compatible. > > typo. "boolean…" ("boolean..." but with U+2026 HORIZONTAL ELLIPSIS instead) is correct UTF-8 in the git commit, but Splinter thinks patches are Latin-1, so you get mojibake. Or if that's not what you meant, please say what my typo was - I can't see one :-) Created attachment 93316 [details] [review] [MC 1/5] Require account plugins to provide the ability to list parameters --- v2: just list the names of parameters, rename the vfuncs to list_*_parameters() and remove the features flags. Created attachment 93317 [details] [review] [MC 2/5] mcp_account_storage_get_flags: add The initial flag is STORES_TYPES, which can be used to decide whether to try to attach types to untyped parameters. Of our built-in plugins, the default keyfile/variant-file storage and the D-Bus test plugin have STORES_TYPES, but the "diversion" plugin does not. Flags are account-specific in case they ever need to vary per-account (e.g. a FROM_TELEPATHY_0 flag might be one way to deal with migration from Telepathy 0.x to 1.0). Also add some convenience API (has_all_flags, has_any_flag) to make code that uses these flags easier to understand. --- Split out from the previous patch. Created attachment 93318 [details] [review] [MC 3/5] _mcd_account_dup_parameters: try to get parameters' types from backend Now that I've deleted ExternalAccountStorage support, we have two uses for this function: * get the parameters to be passed to RequestConnection * get the parameters for our own D-Bus API (PropertiesChanged, GetAll, etc.) For the former, we should know the types already, because we should already have a concrete CM/protocol in mind by the time we get here. For the latter, ideally we shouldn't need the CM's types at all: if the backend is storing parameters with types, it's arguably more correct for Parameters to contain what the user stored, even if that isn't an exact match for what the CM wants. Created attachment 93319 [details] [review] [MC 4/5] _mcd_account_reconnect: if the account isn't valid, just disconnect It is not valid to call _mcd_account_connection_begin() unless the account has a TpProtocol; in particular, if the account is "valid", then we know it does have a TpProtocol. Created attachment 93320 [details] [review] [MC 5/5] Opportunistically migrate accounts from untyped to typed Parameters --- I committed some of the earlier patches out-of-order; these five are the ones that actually had order dependencies. (In reply to comment #24) > (In reply to comment #13) > > [tp-glib master 1/2] tp_connection_manager_param_dup_variant_type: add > > shouldn't you annotate this (transfer full)? > > Er, yes. I assume the telepathy-glib bits are OK with that change? yep. > (In reply to comment #15) > > Humm wouldn't it be easier to return a struct/object then? Something like > > TpConnectionManagerParam so we can have method to check if the param is > > secret or not. > > If we're going to allow for future expansion like this, I think I'd rather > make the vfunc return a list of names (strv of strings, probably) like > get_untyped_parameters() does, and iterate through that calling > get_parameter() for each one - that seems simpler than inventing objects. > Would that be OK from your point of view? Yeah that's fine. Comment on attachment 93316 [details] [review] [MC 1/5] Require account plugins to provide the ability to list parameters Review of attachment 93316 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 93317 [details] [review] [MC 2/5] mcp_account_storage_get_flags: add Review of attachment 93317 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 93318 [details] [review] [MC 3/5] _mcd_account_dup_parameters: try to get parameters' types from backend Review of attachment 93318 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 93319 [details] [review] [MC 4/5] _mcd_account_reconnect: if the account isn't valid, just disconnect Review of attachment 93319 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 93320 [details] [review] [MC 5/5] Opportunistically migrate accounts from untyped to typed Parameters Review of attachment 93320 [details] [review]: ----------------------------------------------------------------- ++ Created attachment 93476 [details] [review] mcd_storage_maybe_migrate_parameters: fix error handling mcp_account_storage_get_parameter() doesn't raise a GError. Created attachment 93477 [details] [review] mcd_keyfile_get_variant: add support for int16, uint16 If we're opportunistically migrating parameters according to CM-specified types, we need to cope with uint16 ('q') for port numbers. Comment on attachment 93476 [details] [review] mcd_storage_maybe_migrate_parameters: fix error handling Review of attachment 93476 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 93477 [details] [review] mcd_keyfile_get_variant: add support for int16, uint16 Review of attachment 93477 [details] [review]: ----------------------------------------------------------------- ++ Fixed in git for 5.17.0 |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.
Quite a long time ago now, Will wrote: /* FIXME: this is ridiculous. MC stores the parameters for the account, so * it should be able to expose them on D-Bus even if the CM is uninstalled. * It shouldn't need to iterate across the parameters supported by the CM. * But it does, because MC doesn't store the types of parameters. So it * needs the CM (or .manager file) to be around to tell it whether "true" * is a string or a boolean… Here is approximately what's left to fix that: * Reimplement parameter type-coercion in terms of tp_variant_convert(), (extending that function if necessary), possibly falling back to how we do it now if tp_variant_convert() fails. tp_variant_convert is reasonably liberal (comparable to tp_asv_get_*), but doesn't do questionable things that we do in MC, like string <-> integer, or really stupid things that we only do because of our GKeyFile history, like string <-> string list. * When we Get(Parameters), if the account has no untyped parameters, return the (typed) parameters with the types we know they have. * Migration step: when an account's CM becomes ready, if it has any untyped parameters, and its backend supports typed parameters, find out their types from the CM and re-save them. * When we Get(Parameters), if the account has untyped parameters and the CM is not ready, make some sort of sensible assumption (for instance use the types from telepathy-spec and Haze, and assume that anything else is a string). If a parameter is stored in type A but the CM expects it in type B, we should try to convert it for the RequestConnection() call, but IMO we should *not* convert the stored value unless/until the user actually calls UpdateParameters.