Crash in hbci online transfer in master due to GList of kvp_frame

[Christian Stimming]

Hi John,

I still have this open issue with the aqbanking online transfer templates in 
the GList of kvp_frame. I now added a unittest that currently fails (if it 
were activated) but needs to be fixed again to get the described feature 
working. You have probably noticed 1ee4210b5f521a66904 - just in case you feel 
like checking the failing assertion yourself... but I think I'll work on this 
sometime during the next weeks. The preferred solution is what you proposed as 
" friend function" below. Thanks!

Regards,

Christian


Am Montag, 7. Juli 2014, 09:33:49 schrieb John Ralls:
> On Jul 6, 2014, at 11:04 PM, Christian Stimming <christian at cstimming.de> 
wrote:
> > Dear John,
> > 
> > thanks for the pointers. It seems like the import-export/aqb use case of
> > kvp was the only place in the code that used them this way, so this is
> > probably why neither you nor anyone else so far has stumbled over this
> > problem.
> > 
> > I can't even easily send you a gnucash file for reproducing this problem,
> > because this very menu item can be called only if libaqbanking can already
> > verify an existing configured online banking setup for this gnucash
> > account. That means even if I send you my gnucash file, the menu items in
> > question won't be active on your machine because you don't have my
> > libaqbanking setup.
> > 
> > The immediate error is that the book's kvp slot "hbci/template-list" (see
> > below for an example section of that slot from my data file) contains a
> > value that is a GList, and those list elements are again kvp slots. When
> > those list elements are asked to be converted from the kvp_value to GList
> > elements in gvalue_list_from_kvp_value(), it calls
> > gvalue_from_kvp_value() but there the "kval" argument is of type
> > KVP_TYPE_FRAME that runs in the PWARN("Error! Attempt to transfer
> > KvpFrame!") and returns NULL, which in turn crashes in
> > kvp_frame.cpp:1677.
> > 
> > I think I was the only one who used this possibility to put a GList of
> > kvp_frames in the data file. At the time in 2003, I needed a GList of
> > GncABTransTempl objects (see src/import-export/aqb/gnc-ab-trans-templ.h
> > and
> > .c) to be stored in the data file. The solution at the time was to put the
> > GncABTransTempl fields into the slots of a kvp_frame (see
> > gnc_ab_trans_templ_to_kvp and gnc_ab_trans_templ_new_from_kvp in that
> > file), and then putting those kvp_frames into a GList and this one in
> > turn in a kvp slot. Yes, I know it was against any form of data hiding,
> > but that's what was there at the time.
> > 
> > However, the data fields don't have to be touched often: This list of
> > GncABTransTempl must be read once when calling the "online transfer" menu
> > item, to populate the list of templates that can be used for the online
> > transfer. (i.e. the bank details for the transfer to "Some Recipient"
> > should be auto-filled when clicking on the item in the template list.)
> > This list is written again when closing the "online transfer" window, in
> > gnc-ab- transfer.c:65 and :66, if and only if the list of available
> > templates has been modified. Which is seldomly the case. Hence, no
> > optimization whatsoever needs to be in place.
> > 
> > How should I access a GList of a structured data type after your kvp
> > changes, when I shouldn't make this a glist of kvp_frames anymore? What
> > would be a good alternative? Thanks a lot in advance!
> 
> Christian,
> 
> This is complicated by the need for compatibility with existing data files.
> Without that concern you could simply encode each GncABTransTempl as a
> GList of string GValues, but that would fail to parse an existing data
> frame.
> 
> One alternative is to add
> 
> /* EFFECTIVE FRIEND FUNCTION */
> extern KvpFrame *qof_instance_get_slots (const QofInstance *);
> 
> to gnc-ab-trans-templ.c and not use the properties mechanism. That's the
> easy way, and for the short term might be the better one because it flags
> the interdependency and requires the least work which may be discarded in a
> year or two.
> 
> Another is to make a GncABTransTempl property in QofBook and make it
> responsible for reading and writing the kvp. That has the unfortunate
> effect of creating a strong interdependency between QofBook and
> GncABTransTempl, which we'd obviously prefer to avoid.
> 
> A third, which heads in the direction I'd like to go with KVP, is to move
> the creation/persistence of GncABTransTempl to the backends. That obviously
> means that it needs to become a child of QofInstance so that it has
> edit/commit/rollback, and so it would be better to wait for that interface
> to be reimplemented in C++ so that you don't have to do it twice on
> GncABTransTempl.
> 
> Regards,
> John Ralls