On Tue, Apr 15, 2008 at 2:36 PM, Charles Day <cedayiv at gmail.com> wrote:
> On Tue, Apr 15, 2008 at 2:11 PM, Derek Atkins <warlord at mit.edu> wrote:
>
> > Quoting Charles Day <cedayiv at gmail.com>:
> >
> > OK, then we should very publicly document the dangerous trade-off: that
> > > the
> > > transaction callback function must never modify the account assigned
> > > to any
> > > split in the transaction, or destroy any split in the transaction, as
> > > doing
> > > so risks a crash.
> > >
> > > If you like, I can add that to the doxygen documentation in Account.h.
> > >
> >
> > Sure. Go ahead. That kind of documentation is a good thing.
>
>
> Will do.
>
>
OK, I've done this and made sure that doxygen will make the warnings fairly
obvious (there is a built-in "warning" format). There actually already were
a couple of warnings in Account.h, but not on functions I was looking at. I
have made sure that now every Account.h function that takes a
TransactionCallback argument has a warning. I'll include these changes with
my QIF importer fix.
> Also, it might make sense to do something like:
> >
> > next = this->next
> > callback(this)
> > this = next
> >
> > instead of:
> >
> > callback(this)
> > this = this->next
> >
>
> That's definitely safer, but wouldn't entirely eliminate the risk of a
> crash since the callback function for the transaction associated with "this"
> split could destroy "next" if they both belong to the same transaction.
>
I went ahead and implemented change this in Account.c, so the loop now looks
like:
for(split_p = priv->splits; split_p; split_p = next) {
/* Get the next element in the split list now, just in case some
* naughty thunk destroys the one we're using. This reduces, but
* does not eliminate, the possibility of undefined results if
* a thunk removes splits from this account. */
next = g_list_next(split_p);
s = split_p->data;
trans = s->parent;
if (trans && (trans->marker < stage)) {
trans->marker = stage;
if (thunk) {
retval = thunk(trans, cb_data);
if (retval) return retval;
}
}
}
This will go in the fix as well, when it's finished and tested.
Or would you prefer the Account.h & Account.c fixes to be submitted
separately from the QIF fixes?
-Charles
qif-to-gnc functionality. But still, there shouldn't be any transactions
> > > > except those you've just imported. You already have a handle on
> > > > those
> > > > transactions so just delete them. Yet I'm still not sure how this
> > > > can
> > > > happen unless the qif-to-gnc is happening too early.
> > > >
> > > >
> > > You can still cancel the import after the qif-to-gnc stage by clicking
> > > Cancel on the final page of the druid instead of Apply. It is also
> > > necessary
> > > to cancel in cases where the qif-to-gnc functionality catches an
> > > exception
> > > (see my fix for bug 509089).
> > >
> > > When the Cancel button event is caught by the C handler, there is no
> > > already-built list of transactions. But either C or Scheme could build
> > > one
> > > based on the root account of the imported tree, which is known.
> > >
> > I also didn't think you could cancel the import after you reach the
> >
> > I see...
> >
> > If I have to build my own list of transactions for a given account
> > > tree, is
> > > a QOF query perhaps the fastest way to go? There is already a routine
> > > written with this strategy in Scheme
> > > (gnc:account-tree-get-transactions in
> > > qif-merge-groups.scm).
> > >
> >
> > Might as well use that existing API. I dont think you need a full
> > query.
> >
>
> OK, I'll give that a shot and see what happens.
>
> Charles
> > >
> > Cheers,
> >
> > -derek
> >
> > --
> > Derek Atkins, SB '93 MIT EE, SM '95 MIT Media Laboratory
> > Member, MIT Student Information Processing Board (SIPB)
> > URL: http://web.mit.edu/warlord/ PP-ASEL-IA N1NWH
> > warlord at MIT.EDU PGP key available
> >
> >
>