[PATCH 0/2] *** SUBJECT HERE ***

[Adrian Perez]

Those two patches improve configuration file handling. One avoids a warning
being printed out when the configuration file does not exists (it will not
be even tried to be parsed when it does not exist) and the second one
ensures that loaded accounts have at least a non-empty token.

I know that a third patch which checks the validity of the token agains the
Flickr API, but I have one question regarding that: what would happen if
a token is *valid* but it cannot be verified because there is no connection
to the Internet at the moment of checking?

What do you think about that possible issue?

Adrian Perez (2):
Check existence of accounts file before parsing
Check for non-empty tokens when loasing accounts

src/frogr-config.c | 12 +++++++++---
1 files changed, 9 insertions(+), 3 deletions(-)

[Adrian Perez]

This avoids an unsightly input/output warning from libxml2, and avoids
calling the XML parser at all if the file does not exist. Also, some
extra checking does not hurt.
---
src/frogr-config.c | 8 +++++++-
1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/src/frogr-config.c b/src/frogr-config.c
index 0519e6c..f07007f 100644
--- a/src/frogr-config.c
+++ b/src/frogr-config.c
@@ -123,7 +123,7 @@ _frogr_config_load_accounts (FrogrConfig *fconfig, const gchar *config_dir)
FrogrConfigPrivate *priv;
gchar *xml_path;
xmlNodePtr node;
- xmlDocPtr xml;
+ xmlDocPtr xml = NULL;

g_return_if_fail (FROGR_IS_CONFIG (fconfig));
g_return_if_fail (config_dir != NULL);
@@ -131,6 +131,12 @@ _frogr_config_load_accounts (FrogrConfig *fconfig, const gchar *config_dir)
priv = FROGR_CONFIG_GET_PRIVATE (fconfig);

xml_path = g_build_filename (config_dir, "accounts.xml", NULL);
+ if (!g_file_test (xml_path, G_FILE_TEST_IS_REGULAR))
+ {
+ g_free (xml_path);
+ return;
+ }
+
xml = xmlParseFile (xml_path);
g_free (xml_path);

--
1.6.2.5

[Adrian Perez]

For an account to be valid, apart from having some non-NULL values (which
was being already checked), check also for the token being non-empty. Other
values but the authentication token might be empty.
---
src/frogr-config.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/frogr-config.c b/src/frogr-config.c
index f07007f..bde126b 100644
--- a/src/frogr-config.c
+++ b/src/frogr-config.c
@@ -100,9 +100,9 @@ _frogr_config_account_from_xml (xmlDocPtr xml, xmlNodePtr rootnode)

/*
* The account object is created only of some minimum requirements are
- * met: user name, token and frob.
+ * met: user name, token and frob. Also, token must *not* be empty.
*/
- if (frob != NULL && token != NULL && username != NULL)
+ if (frob != NULL && token != NULL && username != NULL && token[0] != '\0')
{
faccount = frogr_account_new_with_params ((const gchar *)frob,
(const gchar *)token,
--
1.6.2.5

[Mario Sanchez Prada]

Thanks! Some nitpicking below...

Adrian Perez wrote:
> [...]

> xml_path = g_build_filename (config_dir, "accounts.xml", NULL);
> + if (!g_file_test (xml_path, G_FILE_TEST_IS_REGULAR))
> + {
> + g_free (xml_path);
> + return;
> + }
> +
> xml = xmlParseFile (xml_path);
> g_free (xml_path);
>

I'd re-write this part as follows

> xml_path = g_build_filename (config_dir, "accounts.xml", NULL);

> + if (g_file_test (xml_path, G_FILE_TEST_IS_REGULAR))

> + xml = xmlParseFile (xml_path);

> +
> g_free (xml_path);
>

That way we avoid to duplicate the g_free statement and we also get rid
of that ugly "return"...

What do you think?

Mario

[Mario Sanchez Prada]

Hmmm... be careful, this would lead to a serious problem later on in
case faccount remained NULL by the end of that function. More context:

[...]

if (frob != NULL && token != NULL && username != NULL)

{
faccount = frogr_account_new_with_params ((const gchar *)frob,
(const gchar *)token,

(const gchar *)username,
public, family,friends);
}

if (frob != NULL) xmlFree (frob);
if (token != NULL) xmlFree (token);
if (username != NULL) xmlFree (username);

return FROGR_ACCOUNT (faccount);
}

As you can see, if faccount is NULL the return statement will be
probably mean "no good things" :-). I should have realized of this
before, I know, but to be honest I did not entered to much in detail in
the logic when merging your configuration fwork some months ago... I
just wanted to move forward by those days... :-)

But now we're about to release the first version of frogr it would make
sense to put much attention to these details.

In the other hand, some quick thought related to this:

Would it make sense to have a (private) account validation function
for that, instead of just adding "&& condition" to that if?

Thanks!
Mario

[Mario Sanchez Prada]

Adrian Perez wrote:
> [...]

> I know that a third patch which checks the validity of the token agains the
> Flickr API, but I have one question regarding that: what would happen if
> a token is *valid* but it cannot be verified because there is no connection
> to the Internet at the moment of checking?

I think the right way to go there would be to only decide that a stored
token is not valid when:

- There's no token at all, or not even a well-formed conf file
- We have asked Flickr for the validity of a stored token and we have
received an *explicit* answer telling that it is not valid. I mean,
just getting a generic error while checking it's correcness would
not be enough (could be the network down, DNS problems, Flickr not
reposnding...). It would be needed to check that the proper
*negative* answer from Flickr was got

> What do you think about that possible issue?

I guess that considerations should cover that, but perhaps I'm missing
something here.

Mario

[Adrian Perez de Castro]

I thought about doing it that way, but then when the file does not
exist, the "Could not load %s/accounts.xml" debug message would pop
out, and that message in theory is for when the XML cannot be parsed...

If it is okay for you to show that message when the file does not
exist, your suggestion is good.

:D

--
Adrian Perez de Castro <ape...@igalia.com>
Igalia - Free Software Engineering

[Mario Sanchez Prada]

Adrian Perez de Castro wrote:
> [...]

>> What do you think?
>
> I thought about doing it that way, but then when the file does not
> exist, the "Could not load %s/accounts.xml" debug message would pop
> out, and that message in theory is for when the XML cannot be parsed...
>
> If it is okay for you to show that message when the file does not
> exist, your suggestion is good.

I think it's ok as long as xml_parse get's no called when we know the
file is not present in advance... so 'yes' it would be ok to have that
g_debug.

However, I would bet for not leaving a missing/incorrect/malformed file
at disk when we know at some point (starting up) that is wrong and, more
important, when the app itself would be able to fix that problem by,
say, just deleting or truncating the accounts.xml file.

I know it does not look very elegant, but the true is that at this point
it seems the best and more clean option, and as Frogr only manages 1
user account so far I do not see a big deal with that.

Mario