GNOME Bugzilla – Bug 589266
New SIP account mangles fields if Authentications User is empty
Last modified: 2009-08-01 21:14:14 UTC
In the Accounts window, use Add a SIP account. If you fill the form by letting the Auth user field empty, everything works. However, after stopping ekiga and restarting it, the fields of this account are mangled, as shown by Edit Account window: the password is put in Auth user, and the password and timeout fields have bizarre values.
I suppose it is due to the recent work on accounts.
The supposition the problem is due to my refactorization of the code is based on nothing. Let's dive into the report and try to understand what happens. It's a problem with the action of adding a SIP account, so this action is provided by the populate_menu in opal-bank.cpp, obviously -- the engine does make some sense. Looking at that code, we see the action is implemented by the 'new_account' method, which works with a form, handled in the 'on_new_account_form_submitted' method (by the way, at that point we see that it's the proper place to detect errors like duplicate accounts... and that it already does some checking, with a nice and simple get-back-to-the-user piece of code -- eh, it looks like forms where thought to make that easy!). It basically just calls the 'add' method. The 'add' method just calls a constructor of Opal::Account, so we dive in opal-account.cpp and read there... The constructor which is called by Opal::Bank::add is the second in the file, and it surprisingly only initializes instance data : where is it stored for the next run ?! There's an 'as_string' method, which does a primitive serialization ("primitive" means it will break when a new field is added, or when a user will have the idea to choose "|337" as his password), so we are now looking for a 'from_string' method or some such. Ah! It's the first constructor which does it, using strtok -- which means the unserialization is as bad as the serialization. At that point, I can make two claims, based on evidence : (1) I didn't break anything ; (2) the original code is broken by non-design. Damien, could you please stop saying more or less covertly I'm breaking everything I touch and making things needlessly complex?
Bug #585052 has additional information, maybe both can be fixed by one shot.
Indeed, it's a duplicate. *** This bug has been marked as a duplicate of 585052 ***
The problem is GConf not being very useful with that kind of structure to store. The | thingy was used since ages in Ekiga 2.x, and was kept for Ekiga 3.0. I think it should be rewritten to store pure XML. The problem being that we need to write an upgrade routing converting from the old format to the new one. (that is the boring part).
Do you know other places where the '|' trick is still used? I was pretty sure we had got rid of all this. The upgrade from old to new isn't boring : it's tricky and error-prone.
The output of: greprsvn -r '|' ekiga-svn/|grep -v '||'|less shows that it only appears (for our purposes) in: ekiga-svn/lib/engine/components/opal/opal-account.cpp: char *pch = strtok ((char *) account.c_str (), "|"); ekiga-svn/lib/engine/components/opal/opal-account.cpp: pch = strtok (NULL, "|"); ekiga-svn/lib/engine/components/opal/opal-account.cpp: str << enabled << "|1|" ekiga-svn/lib/engine/components/opal/opal-account.cpp: << aid << "|" ekiga-svn/lib/engine/components/opal/opal-account.cpp: << name << "|" ekiga-svn/lib/engine/components/opal/opal-account.cpp: << protocol_name << "|" ekiga-svn/lib/engine/components/opal/opal-account.cpp: << host << "|" ekiga-svn/lib/engine/components/opal/opal-account.cpp: << host << "|" ekiga-svn/lib/engine/components/opal/opal-account.cpp: << username << "|" ekiga-svn/lib/engine/components/opal/opal-account.cpp: << auth_username << "|" ekiga-svn/lib/engine/components/opal/opal-account.cpp: << (password.empty () ? " " : password) << "|"
You should also check the uses of g_strsplit for places where things could be hairy ; if we did things right, the only such place is gmconf-glib.c, and it has code to deal with it as properly as possible -- code which has been pretty painful to write, but has been debugged since long. I'm pretty confident the only place where something like this is done is the opal account code. PS: there are other uses of g_strsplit, but they shouldn't be "hairy" ; it should be places where there can be no user entering input which will lead to bad mangling.
I do not think this is a duplicate of the bug for removing |. We could fix this bug and let the other for later. I noticed that if Auth user is empty, a duplicate | (i.e. "||") is found in the string, so the problem is somehow afterwards. I will analyse this further.