Subject: RE: FvwmIdent on Tru64 reports interesting X and Y coordinates...

From: Jeremie Petit <[email-masked]>
Date: Thu, 27 Apr 2000 18:06:02 +0100

Hello dear fvwm workers,

  is there anything new about this problem ? Shouldn't it belong to
Dominik's TODO list before 2.4 ?

  Thanks,

	Jeremie Petit.

		-----Original Message-----
		From:	Bob Woodside [mailto:[email-masked]]
		Sent:	Saturday, February 05, 2000 8:43 PM
		To:	Dan Espen
		Cc:	[email-masked]
		Subject:	Re: FvwmIdent on Tru64 reports interesting X
and Y coordinates...

		Dan --

			Sorry it's taken so long for me to get around to
reading this. Sadly, I
		haven't had much free time for working on FVWM lately. Just
keeping up
		with the mail has been a big deal.

		Dan Espen wrote:
		> 
		> 
		> Heres how I think it works:
		> 
		> The numbers come from an M_CONFIGURE_WINDOW packet sent by
fvwm.
		> The packet is built in fvwm/module_interface.c by the
		> CONFIGURENEW macro:
		> 
		>             (unsigned long)(sizeof(unsigned long)),\
		>             &(*(_t))->boundary_width,\
		> 
		> Bob,
		> 
		>   Isn't this code saying that boundary_width is stored as
a long
		> in the FvwmWindow structure?  (Its an int.)

			Yes, unfortunately. When I wrote the
make_new_vpacket code for the GSFR
		I was blithely assuming that everything pointed to by the
CONFIGARGS
		macro was a long, based on this snippet from the old
make_vpacket, where
		it's actually moving the args into the packet:

			  for (; num > 0; --num)
			    *(bp++) = va_arg(ap, unsigned long);

		> Does "make_new_vpacket" handle the conversion of ints to
longs?
		> I don't quite follow it.  Yet.

			No. The purpose of make_new_vpacket was to allow us
to build an
		arbitrary structure which the receiving module would
reference via a
		structure definition in vpacket.h, rather than referencing
variables via
		the old "body[6]" style syntax - all this so we could pass
the new GSFR
		flags structure. So make_new_vpacket just moves in fields
for the length
		you specify, with no notion of data types. The only special
thing it
		does is to round the total packet size up to a multiple of
the size of
		an unsigned long, for the benefit of the sending/receiving
code.

		> I would think that more than FvwmIdentify is going to be
affected.

			Much more - everyone that looks at the
M_CONFIGURE_WINDOW packet is
		potentially affected on 64-bit machines, and at least has to
be looked
		at.

		> I think the code in question should be more like:
		> 
		> Before:
		>             (unsigned long)(sizeof(unsigned long)),\
		>             &(*(_t))->title_g.height,\
		>             (unsigned long)(sizeof(unsigned long)),\
		>             &(*(_t))->boundary_width,\
		> After:
		>             (int)(sizeof(_t->title_g.height)),\
		>             &(*(_t))->title_g.height,\
		>             (int)(sizeof(_t->boundary_width)),\
		>             &(*(_t))->boundary_width,\

			Not quite...

			c/(int)(sizeof(_t->title_g.height))/(unsigned
		long)(sizeof(_t->title_g.height))

		etc.

			The length arguments passed to make_new_vpacket must
be unsigned longs
		- the field-length argument is only used by
make_new_vpacket, it doesn't
		go into the packet.

			The problem is that the fields are getting stuffed
into the packet with
		an incorrect length, which would be easy enough to fix, in
the way you
		suggested above (with my correction). Unfortunately this
would still
		leave us with the problem that everyone expects these fields
to be long
		ints in the packet, and there's no provision for making this
conversion,
		so we'd still have the packet out of sync with what the
modules expect.
		Actually, it would be more so, since all the things that
really are
		longs, and are currently aligned properly, would now be
misaligned as
		viewd by the modules.

			This is true no matter whether the modules use the
old "body[n]"
		technique or use the ConfigWinPacket struct in vpacket.h;
since I was
		stupidly assuming that everything in this packet was a long
(except for
		the new window_flags struct, which was the whole reason for
this
		change), the ConfigWinPacket struct is incorrect as well.

		> 
		> Pass the size args as ints, shorts or chars would work
too.
		> Get the value for sizeof from the field itself.
		> 
		> Or perhaps, just cast everything to a long in the macro.

			Unfortunately, since the actual fields are being
passed by pointer
		reference, we can't play games with casting the actual data
args (which
		I believe is what the old make_vpacket did). But something
that has the
		same effect may be the best way to go.

		> 
		> But this won't work by itself, more changes are needed.
		> 
		> (I'm just trying to convey the problem at this point.)

			I'll have to give this some thought. Several things
come to mind that
		we might try.

			1) Change all of the FvwmWindow int items that are
sent in the
		M_CONFIGURE_WINDOW packet to longs. This would require a
good bit of
		double checking for new type mismatch problems in the core
fvwm code,
		but the packet structure would be back in sync and the
modules should
		not need changing.

			2) Replace the CONFIGARGSNEW macro technique of
invoking
		make_new_vpacket with a bit of code (or wrap the call with a
bit of
		intermediate code) that would convert all the int items to
longs before
		calling make_new_vpacket. This may be the best way to go.

			3) Redefine the make_new_vpacket API so that there
is some way of
		telling it that a particular argument is an int that needs
to be forced
		to a long. I'm not too wild about this one - it could get
really ugly.

			4) Do a major investigation of all the modules that
use
		M_CONFIGURE_WINDOW to see how much trouble it would be to
fix them to
		use the packet as it would be sent with the correction above
to use the
		actual lengths of the int fields. The problem I see with
this approach
		is that it would definitely break the old binary
compatibility that
		several people spent a lot of time and effort to try to
maintain (i.e.,
		the idea was that everything in the packet *before* the new
window_flags
		struct was supposed to continue to look just the way it
always did).

			Now if only I had one of those Alphas to test on....
:-)

		Cheers,
		Bob
		--
		Visit the official FVWM web page at
<URL:http://www.fvwm.org/>.
		To unsubscribe from the list, send "unsubscribe
fvwm-workers" in the
		body of a message to [email-masked].
		To report problems, send mail to [email-masked]
--
Visit the official FVWM web page at <URL:http://www.fvwm.org/>.
To unsubscribe from the list, send "unsubscribe fvwm-workers" in the
body of a message to [email-masked].
To report problems, send mail to [email-masked]

---

From: Michael Han <[email-masked]>
Date: Sat, 29 Apr 2000 17:09:46 -0700

Previously from Jeremie Petit
>Dominik's TODO list before 2.4 ?

I think this should be fixed prior to any 2.4 release. I'm guessing
others agree.

>Previously from Bob Woodside:
>[snip]
>2) Replace the CONFIGARGSNEW macro technique of invoking
>make_new_vpacket with a bit of code (or wrap the call with a bit of
>intermediate code) that would convert all the int items to longs before
>[snip]
>4) Do a major investigation of all the modules that use
>M_CONFIGURE_WINDOW to see how much trouble it would be to fix them to
>use the packet as it would be sent with the correction above to use the
>actual lengths of the int fields. The problem I see with this approach
>is that it would definitely break the old binary compatibility that
>several people spent a lot of time and effort to try to maintain (i.e.,
>the idea was that everything in the packet *before* the new window_flags
>struct was supposed to continue to look just the way it always did).

Well, grepping for CONFIGURE I find that about half the modules that
care about M_CONFIGURE_WINDOW packets are already using vpacket.h.
Exceptions I see: FvwmIconMan, FvwmIdent, FvwmDebug, FvwmSave,
FvwmSaveDesk. I don't see that it should be too difficult to convert
these to use the ConfigWinPacket struct in libs/vpacket.h. Of course,
there are still non-core modules like FvwmKb (it doesn't care about
CONFIGURE and ADD) and TkGoodStuff.

Personally, I think converting the core modules to refer to vpacket.h
is a good thing no matter what, so I've done an ugly hack in
FvwmIconMan/fvwm.c to make it use vpacket.h and ConfigWinPacket.
Fortunately, Brady had written it so that he'd already setup IconMan
to treat the body as a struct, so conversion was easy. Unfortuantely,
it looks messy because ConfigWinPacket doesn't really "fit in" with
the rest of the code... Still, I compiled it -Wall without trouble and
it seems to work fine. See attachment for the diff -u. If there's
interest, I can go through the other modules and convert them too.
-- 
[email-masked]                         http://www.best.com/~mikehan/
coffee achiever                              San Francisco, California
Avoid cliches like the plague
--
Visit the official FVWM web page at <URL:http://www.fvwm.org/>.
To unsubscribe from the list, send "unsubscribe fvwm-workers" in the
body of a message to [email-masked].
To report problems, send mail to [email-masked]