Re: [XBoard-devel] [PATCH 1/2] Fix segfault with 'Game List Tags' dialogue

[Thomas Adam]

On Sat, Mar 21, 2015 at 11:48:58AM +0100, H.G. Muller wrote:
> 
> 
> Thomas Adam schreef op 3/21/2015 om 12:06 AM:
> >
> >I see---it seems a little... hmm, odd, that the presense of the dialogue
> >is the thing to check.  That there is no data in glc seems the more
> >technically correct thing to check for.
> >
> I think the two are always correlated, as XBoard always created the dialog
> as soon
> as it gets a non-empty game list. I guess at the time it felt natural to
> test for this,
> because I assumed that the actual segfault was due to accessing a
> non-existent widget.
> So I added a test for its existence, I could have been wrong about that,
> though.

Oh that's fine, it's just a little odd to test for this based on a GUI
widget rather than the backend data (which ultimately drives the widget's
ability).  It's fine the way you have it, although if that situation were to
ever change, then the check would also have to change; hence why looking at
the data structure gives a little more robustness.  But ultimately it's a
moot point, at least it's fixed.  :)

> >It bothers me, because it's crufty, and often that looks bad, especially
> >in terms of code structure, etc.
> Well, I am not saying that this should not be done. I just explained I am
> not very motivated
> to do it. I am basically only interested in adding new functionality, and
> then usually because
> it is functionality I need myself as an XBoard/WinBoard user. So it is all
> the better if there
> are other people that would be interested in cleaning up the code. (Which
> was already in
> a pretty sorry state by the time I got involved.)

That's understandable.  Of course, the motivation from my point of view
(which you could argue is something akin to yak-shaving), is that it will
make contributing new features slightly easier, if the state of the
surrounding code is a little more consistent, if that makes sense?

> The FREE and ASSIGN macros are things I added myself relatively recently,
> because I got
> a bit tired from writing "if(f) free(f); f=dup(x);" all the time. So in new
> code I use those.

That's OK, their use can always be expanded as the surrounding code is
updated, etc.  That'll be a good change to adopt them.

> But all the old code still writes it explicitly; I did not see any point in
> replacing working code,
> as thie macros were intended to save me work, not produce it.
>   It would be good to also have a macro DEBUG for the construct
> "if(appData.debugMode") fprintf(debugFP, ", which is also something I have
> to type
> annoyingly often.

There's a few ways of solving this.  You could have a log_debug() function
or something which does this, perhaps even a runtime toggle?  Who knows.

> I wasn't even aware something like asprintf existed. I don't know if there
> is a particular
> reason avoiding it. (Like compatibility with old libraries that do not have
> it. Amazingly
> enough even snprintf seems to give problems in MicroSoft Visual Studio.) In
> code I add
> I usually print to declare static or automatic arrays, rather than to
> allocated memory.

That sort of change is the more useful one, to reduce the malloc/snprintf
code-churn.  I had a look at this, and portable code can always provide its
own implementation if it's not found via configure, etc.  I'm prototyping
this at the moment.

> At one point we tried to replace all strcpy by safeStrCpy. But being
> dogmatic about that
> often leads to very silly code, like safeStrCpy(*command, 512, "go\n");
> where there is
> no doubt at all that the 3 printed characters will fit in the target, and
> you have to assume
> the size of the target anyway, because it is only available as (char*).

Uh huh.  I would strongly suggest strlcpy() though.

> Well, this is a problem. Anything change in the back-end that would require
> a compensating
> change in the XBoard front-end are likely to break WinBoard, if the WinBoard
> front-end
> would not get the compensating change too. A weaker problem is functional
> equivalence;
> it is undesirable if features added to XBoard are not operational in
> WinBoard for lack of
> front-end functionality, even though they compile OK.

I understand that, and I'd obviously do my best.  But without a means of
testing this, I'm at a very loose-end indeed over this, and obviously do not
wish to break Winboard as a result.

The winboard/makefile.gcc file suggests that it can be cross-compiled, yet
it's clearly not received any love for sometime, since there's calls to
-mno-cygwin which have been deprecated options to GCC---and in more recent
versions have been completely removed (hence 'make -f makefile.gcc' now
errors out).

So although I could install the msys toolchain on my workstation, I am still
dubious whether I'll be able to cross-compile at all.  Even if that were
successful somehow, is this testable under wine or some such, or are such
tests misleading?

It sounds to me from what you're saying that the coupling between the
backend and Winboard might still be a little too tight---that is, how much
of the WIN32 code has actually been shifted into winboard itself?
Obviously, changes to the backend, if that separation were completely true,
ought to reduce the chances of breaking Winboard.

I don't like the thought of developing for XBoard, knowing that it has the
ability to break Winboard, so anything you can suggest to me to reduce this
likelihood is much appreciated.

Thanks!

-- Thomas Adam