I can bring the network up in a bash session.  However, anaconda cannot bring the network up.

We thought that it might be a virtual ethernet problem.  However, we installed a physical ethernet card in the system.  It still fails.

I see no error regarding NetworkManager, there's a successful connection activation in the log.
However, there is an error with memory access in ibmveth driver, thus reassigning to kernel.

The error you are talking about already has a bug assigned to it.  bug 733766.

I am able to bring the network up successfully from the command line using ifup.  I cannot bring the network up using the Anaconda installer (which uses the NetworkManager libraries).

Created attachment 915366 [details]
Comment

(This comment was longer than 65,535 characters and has been moved to an attachment by Red Hat Bugzilla).

Created attachment 915367 [details]
Comment

(This comment was longer than 65,535 characters and has been moved to an attachment by Red Hat Bugzilla).

Is there any chance we can get anaconda output for this error?  Based on these dmesg logs, it appears as though NetworkManager is correctly configuring the device, and that the device does indeed have IP connectivity (as shown by the resulting ifconfig output).  But the bug title indicates that anaconda is saying that the network was not correctly configured?  Is that correct?

Also, what does 'route -n' say?

[anaconda root@localhost dev]# ifup eth1
[anaconda root@localhost dev]# route -n
Kernel IP routing table
Destination     Gateway         Genmask         Flags Metric Ref    Use Iface
0.0.0.0         9.5.250.1       0.0.0.0         UG    0      0        0 eth1
9.5.250.0       0.0.0.0         255.255.255.0   U     0      0        0 eth1
169.254.0.0     0.0.0.0         255.255.0.0     U     1003   0        0 eth1
[anaconda root@localhost dev]# /bin/systemctl start loader2.service

This brings up the anaconda installer.  I see the text dialog saying "You have multiple network devices on this system. Which would you like to install through?"

I select eth1 and then see the following text dialog: "Waiting for NetworkManager to configure eth1."

Which displays for a while.  I then see the dialog to check the media ("To begin testing the media before installation press OK.").  Which I skip, and it goes back to "Waiting for NetworkManager to configure eth1."  After a while it goes to the "Configure TCP/IP" dialog.

The network is still up from when I started it before.  I can ssh into the box and I only see these logs from anaconda:

[anaconda root@localhost ~]# ls -l /tmp
total 5
-rw-r--r-- 1 root root 3601 Sep 14 19:53 anaconda.log
-rw-r--r-- 1 root root   98 Sep 14 19:51 program.log
[anaconda root@localhost ~]# cat /tmp/program.log
19:51:13,711 INFO program: Running... /bin/mount -n -t iso9660 -o ro /dev/sr0 /mnt/install/source
[anaconda root@localhost ~]# cat /tmp/anaconda.log
19:48:39,815 INFO loader: kernel command line:
19:48:39,815 INFO loader:     vnc=1
19:48:39,815 INFO loader: vnc forced graphical mode from cmdline
19:48:39,815 INFO loader: early networking required for vnc
19:48:39,815 INFO loader:     ro
19:48:39,815 INFO loader:     selinux=0
19:48:39,815 INFO loader:     systemd.log_target=kmsg
19:48:39,815 INFO loader:     systemd.log_level=debug
19:48:39,815 INFO loader:     rd.break
19:48:39,815 INFO loader:     serial
19:48:39,815 INFO loader:     root=live:CDLABEL=Fedora-20110907-ppc64-DVD
19:48:39,817 DEBUG loader: readNetInfo /tmp/s390net not found, early return
19:48:39,831 INFO loader: anaconda version 16.16 on ppc64 starting
19:48:39,832 INFO loader: set console to /dev/hvc0 at 108
19:48:39,850 INFO loader: 8519680 kB (8320 MB) are available
19:48:39,850 INFO loader: 8519680 kB (8320 MB) are available
19:48:40,106 DEBUG loader: going to set language to en_US.UTF-8
19:48:40,106 DEBUG loader: locale en_US.UTF-8: base: en_US, mod: (null), charset: UTF-8
19:48:40,107 INFO loader: going to prepare locales for en_US.UTF-8 (locale: en_US, charset: UTF-8)
19:48:41,266 INFO loader: setting language to en_US.UTF-8
19:48:41,269 DEBUG loader: Saving module scsi_dh_rdac
19:48:41,269 DEBUG loader: Saving module scsi_dh_hp_sw
19:48:41,269 DEBUG loader: Saving module scsi_dh_alua
19:48:41,269 DEBUG loader: Saving module scsi_dh_emc
19:48:41,269 DEBUG loader: Saving module iscsi_tcp
19:48:41,269 DEBUG loader: Saving module libiscsi_tcp
19:48:41,269 DEBUG loader: Saving module libiscsi
19:48:41,269 DEBUG loader: Saving module scsi_transport_iscsi
19:48:41,269 DEBUG loader: Saving module cramfs
19:48:41,269 DEBUG loader: Saving module squashfs
19:48:41,269 DEBUG loader: Saving module nls_utf8
19:48:41,270 DEBUG loader: Saving module ibmvscsic
19:48:41,270 DEBUG loader: Saving module scsi_transport_srp
19:48:41,270 DEBUG loader: Saving module scsi_tgt
19:48:41,270 DEBUG loader: Saving module e1000e
19:48:41,270 DEBUG loader: probing buses
19:48:41,411 DEBUG loader: waiting for hardware to initialize
19:48:47,870 INFO loader: restarting NetworkManager
19:48:48,418 INFO loader: No iBFT table detected.
19:50:28,662 INFO loader: doing kickstart... setting it up
19:50:28,664 DEBUG loader: activating device eth1
19:51:13,704 ERR loader: failed to configure network interface
19:51:13,704 ERR loader: unable to activate device eth1
19:51:13,706 INFO loader: trying to mount CD device /dev/sr0 on /mnt/install/source
19:51:13,707 INFO loader: drive status is CDS_DISC_OK
19:51:13,719 DEBUG loader: Found installation media, so skipping lang and kbd
19:53:03,928 DEBUG loader: in doLoaderMain, step = STEP_LANG
19:53:03,928 DEBUG loader: in doLoaderMain, step = STEP_KBD
19:53:03,928 DEBUG loader: in doLoaderMain, step = STEP_METHOD
19:53:03,928 DEBUG loader: in doLoaderMain, step = STEP_DRIVER
19:53:03,930 DEBUG loader: in doLoaderMain, step = STEP_NETWORK
19:53:03,965 INFO loader: need to set up networking
19:53:03,966 DEBUG loader: in doLoaderMain, step = STEP_IP
19:53:03,966 DEBUG loader: in doLoaderMain, calling setupIfaceStruct()
19:53:03,966 DEBUG loader: in doLoaderMain, calling readNetConfig()
19:53:03,966 INFO loader: doing kickstart... setting it up
19:53:03,967 DEBUG loader: activating device eth1
19:53:49,010 ERR loader: failed to configure network interface
19:53:49,010 DEBUG loader: in STEP_IP, retry (LOADER_ERROR)
19:53:49,010 DEBUG loader: in doLoaderMain, step = STEP_IP
19:53:49,010 DEBUG loader: in doLoaderMain, calling setupIfaceStruct()
19:53:49,010 DEBUG loader: in doLoaderMain, calling readNetConfig()

I'm confused.  At what point here does the error this bug is about occur?  At no point should 'ifup eth1' be necessary, and I assume you're doing that because you got the aforementioned error?  Why is ifup being run manually at all, and at what point during the install are you running ifup?

Basically, what I need here is ifconfig -a and route -n after the error has occurred, but *before* any additional action like 'ifup' has been taken.  Otherwise we simply pollute the debugging results.

From the logs, it looks like NM is doing everything correctly and setting up the interface.  It shouldn't be necessary to run 'ifup' because networking is apparently being set up correctly by NetworkManager already.

Boot Fedora-20110907-ppc64-DVD-respin.iso with the following parameters:
    linux serial vnc=1 selinux=0

You will immediately run into the problem.

I use an HMC to access the console.  Unfortunately, I cannot access the other windows that anaconda creates.  So, I create the network setup files manually and start the network before anaconda runs.  This allows me to get into a bash window and poke around.

http://ppc.koji.fedoraproject.org/scratch/karsten/iso/ is the URL where the isos can be found.

Some observations about logs in comment #5:

1) systemd starts NM

2) NM finds there is no defined connection for eth0 or eth1, and creates some default DHCP connections and starts them up

3) these default connections successfully start at time 99

4) at time 274, NM is told to stop

5) at time 275, NM is told to start again

6) NM finds new config files ifcfg-eth0 and ifcfg-eth1, but these contain NM_CONTROLLED=no so NM does not start any network interfaces

7) at 278, anaconda is supposed to flip NM_CONTROLLED to 'yes' to let NM start the interfaces, as indicated by:
[  278.609893] loader[745]: activating device eth1

8) NM never gets an inotify signal indicating that the files have changed, thus it never starts the interfaces.  If NM got the inotify signal, we'd expect to see a message like:

NetworkManager: updating /etc/sysconfig/network-scripts/ifcfg-eth0


One other possibility is that the code in NM's config file parsing for handling NM_CONTROLLED isn't properly parsing the change from 'no' to 'yes'.  There were some changes committed around 2011-08-07 to that part of the code, but they were supposed to fix an issue like this.  I just checked and verified that flipping NM_CONTROLLED on my machine with the official NM 0.9 release (in F15+) works as expected, so I don't suspect this change yet.

One way to test out whether it's anaconda or NM or the kernel is to:

1) after you get the "failed to configure interface" error, do whatever you need to do to get shell access to the machine's filesystem

2) check /etc/sysconfig/network-scripts/ifcfg-eth0 and ifcfg-eth1 and see what the value of NM_CONTROLLED is

3) change that value to the opposite, ie "no" -> "yes" or "yes" -> "no"

4) look at the last 50 lines of syslog or wherever the NM output goes, and see if NM noticed the change to the ifcfg file

/etc/sysconfig/network-scripts/ifcfg-eth0 is empty after NM failed to configure eth0:


DEVICE=eth0
HWADDR=C6:.....
ONBOOT=yes
BOOTPROTO=dhcp
IPV6INIT=yes
IPV6_AUTOCONF=yes


That was with an aborted anaconda, I need to try again and see if I can only suspend it while doing the change

Before NM runs, /etc/sysconfig/network-scripts/ifcfg-eth0 is:

DEVICE=eth0
HWADDR=2A:....
ONBOOT=yes
BOOTPROTO=dhcp

After it fails to configure, it's:

DEVICE=eth0
HWADDR=2A:....
ONBOOT=yes
BOOTPROTO=dhcp
IPV6INIT=yes
IPV6_AUTOCONF=yes

I added NM_CONTROLLED=yes to /etc/sysconfig/network-scripts/ifcfg-eth0 and hit "Retry" in Anaconda and it just overwrote the file.

Yeah, anaconda will rewrite the file when trying to configure the  network, that's expected.  But after you manually modified ifcfg-eth0 and added NM_CONTROLLED=yes, did you see anything in syslog that indicated NM reread the file?  If not, try changing IPV6INIT to "no" and see if that triggers a reread.

From further debugging with modified NM versions it looks like NM is only getting glib's CHANGED event for file change notifications, and not CHANGES_DONE_HINT.  This could be due to changes in either glib's file monitor support or the kernel.

One thing I noticed is that glib's virtual changes done hint doesn't appear to be firing.  It has a 2-second timer to send the changes done hint but that apparently never happens.  A second odd thing is that we'd expect inotify to send an IN_CLOSE_WRITE event when the modified file is closed for writing, and that gets sent through directly by glib, but that's also not happening.

SO the next step is probably to build a glib2 package with some debug logging and figure out what's going on.

So after much debugging on the part of rangerpb, hamzy, and I, it comes down to libffi as the culprit.  glib now uses libffi to magically figure out the marshalling for signal arguments.  Apparently that doesn't work correctly on ppc64, as the following log shows:

18:25:31,0 INFO NetworkManager: GLib-GIO-Message: (730) emit_cb (0x100205378b0): path '/etc/sysconfig/network-scripts/ifcfg-eth0' change 0x1002059fa20 event 3 (other <none>)
18:25:31,0 NOTICE NetworkManager:    ifcfg-rh: (0x100205378b0) path changed /etc/sysconfig/network-scripts/ifcfg-eth0 (other <none>) event 0

The first part is from glib/gio/gfilemonitor.c's emit_cb() function where it does this:

       g_signal_emit (monitor, signals[CHANGED], 0,
	  	      change->child, change->other_file, change->event_type);

while the second simply prints out the signal arguments that NetworkManager got.  Note that NetworkManager receives an event type of '0', while glib is emitting an event type of 3.  That's the core problem here.

At the moment we blame libffi, but we need to run both the libffi and glib test suites on a ppc64 box before we can more definitively say where the problem lies.

Just for the record, the glib signal arguments here are:

  signals[CHANGED] =
    g_signal_new (I_("changed"),
		  G_TYPE_FILE_MONITOR,
		  G_SIGNAL_RUN_LAST,
		  G_STRUCT_OFFSET (GFileMonitorClass, changed),
		  NULL, NULL,
		  NULL,
		  G_TYPE_NONE, 3,
		  G_TYPE_FILE, G_TYPE_FILE, G_TYPE_FILE_MONITOR_EVENT);

and the G_TYPE_FILE_MONITOR_EVENT is the one that's apparently not getting marshalled appropriately.  That GType just represents an enum/uint32.

Which version of libffi? 3.0.10?

Created attachment 524478 [details]
reduced testcase for marshalling G_TYPE_FILE_MONITOR_EVENT

Build with:

gcc -Wall -o fmtest `pkg-config --libs --cflags gio-2.0` fmtest.c

this test passed on x86_64, fails on ppc64.

testcase requires only glib2-devel package installed to build.

libffi-3.0.9-3.fc16.ppc64
libffi-devel-3.0.9-3.fc16.ppc64

Note that I don't think we can rule glib2 out completely yet...  still investigating glib's usage of libffi and the internal mapping of types for marshalling.

G_TYPE_ENUM (which is what G_TYPE_FILE_MONITOR_EVENT is derived from) was not correctly mapped to libffi types during marshalling in gclosure.c.  Internally it was stored as a 'signed long' but when mapped to libffi types, was retrieved as a 'signed int' leading ppc64 to always return 0 since it presumably got the wrong 32 bits of the union in which the mapped value was held.

(In reply to comment #25)
> G_TYPE_ENUM (which is what G_TYPE_FILE_MONITOR_EVENT is derived from) was not
> correctly mapped to libffi types during marshalling in gclosure.c.  Internally
> it was stored as a 'signed long' but when mapped to libffi types, was retrieved
> as a 'signed int' leading ppc64 to always return 0 since it presumably got the
> wrong 32 bits of the union in which the mapped value was held.

Great - thanks for tracking this down.

I've built a custom glib2 with upstream commits 1df8160fa675b225809eed2f86d2489133e5e54d and
f42fe6cdc056b77f74ff6e332389d444c50ae7dc

the fmtest testcase still fails with
** Message: a_foo_emit: emitting signal with G_FILE_MONITOR_EVENT_CREATED (3)
**
ERROR:fmtest.c:62:check_cb: assertion failed: (event_type == G_FILE_MONITOR_EVENT_CREATED)



The glib2 selfcheck fails with:
TEST: signals... (pid=21717)
  /gobject/signals/variant:                                            OK
  /gobject/signals/generic-marshaller-1:                               **
ERROR:signals.c:193:on_generic_marshaller_2: assertion failed (v_enum == TEST_ENUM_BAR): (0 == 2)
OK
  /gobject/signals/generic-marshaller-2:                               FAIL
GTester: last random seed: R02S8fe9c3d2d27713f79a52c55453f3343b
/bin/sh: Zeile 1: 21656 Beendet                 MALLOC_CHECK_=2 MALLOC_PERTURB_=$((${RANDOM:-256} % 256)) ../../glib/gtester --verbose boxed enums param signals threadtests dynamictests binding properties reference ifaceproperties valuearray
make[4]: *** [test-nonrecursive] Fehler 143

In gobject/genums.c, there also seems to be mismatched types.

Also, since enums are ints in C, shouldn't we be storing them in ints rather than longs?

(In reply to comment #28)
> In gobject/genums.c, there also seems to be mismatched types.
> 
> Also, since enums are ints in C, shouldn't we be storing them in ints rather
> than longs?

Not storing the enums in v_long would be an ABI break.  I did a patch to stuff them into a temporary integer but that produced signed/unsigned issues with enums instead.  Better, but still not fixed.  The patch I had works fine on ppc32, just not ppc64.  Are there sign extension rules for storing an int to a long in ppc64 that might be coming into play?

nothing more than any other arch :-)

If you store as an int you need to retrieve as an int, that's the one cardinal rule. IE, retrieve with the same type you used to store.

As for casting before you store, that's business as usual, ie, int -> long -will- sign extend, if you want to avoid that, you need to treat things as unsigned int -> unsigned long

(In reply to comment #30)
> nothing more than any other arch :-)
> 
> If you store as an int you need to retrieve as an int, that's the one cardinal
> rule. IE, retrieve with the same type you used to store.
> 
> As for casting before you store, that's business as usual, ie, int -> long
> -will- sign extend, if you want to avoid that, you need to treat things as
> unsigned int -> unsigned long

The patch we've come up with works so far.  What's not working is marshalling return values.  libffi is told that the value is a 32-bit value (ffi_type_sint) yet apparently when it comes back from ffi_call() it's been munged into a 64-bit value.  I would have expected a 32-bit value back from ffi_call, and everything I can see in ffi says that a 4-byte value got allocated on the stack for the return value, but on return it's not.

g_cclosure_marshal_generic:

  ffi_call (&cif, marshal_data ? marshal_data : cc->callback, rvalue, args);

(rvalue is a void* pointer to a stack-allocated signed int here)

but if we expect a return value of -30 and do this after ffi_call():

fprintf (stderr, "int %d\n", *(gint*)rvalue);
fprintf (stderr, "long %ld\n", *(glong*)rvalue);

we get output of:

int -1
long -30

where the -30 is really 0xFFFFFFFFFFFFFFE2 underneath.  So the question of the day is why libffi appears to be returning a 64-bit value when were requested a 32-bit signed int.

Created attachment 525010 [details]
Hackish patch which fixes marshalling on ppc64 and has no regressions on x86-64

This patch passes the testcases on ppc64 and x86-64, but I'm really not sure why we need to do the return munging in value_from_ffi_type() since we're telling libffi that we expect a return type of 'sint' which is a 4-byte value, but it appears libffi is returning a pointer to a 8-byte/64-bit value instead.

IMHO that's wrong. Instead you should marshall G_TYPE_ENUM to a full long (arguably an unsigned one even).

The target code will expect all arguments to be fully zero or sign extended to 64-bit anyways, whether they are passed in registers or on the stack, and your g_value as v_long is already appropriately extended by glib, so just marshall it as a long.

Of course I have no way to tell whether that will work on not on x86_64, sparc64, mips64, ia64, ... tho I wouldn't be surprised is sparc is just like powerpc here

Created attachment 525015 [details]
Untested patch to fix 32-bit return values in libffi

So from my quick look at it, it appears that libffi doesn't properly deal
with being asked for a 32-bit return value, it always returns a 64-bit
value on ppc64. I believe this is a bug.

This patch uses the FLAG_RETURNS_64BITS that is set by the prep code to
decide whether to use a stw or a std instruction for the return value,
which should -hopefully- fix it.

Untested here so let me know.

For 64-bit ABIs that extend integral returns types to 64-bits, libffi always returns full 64-bit values that you can truncate in the calling code.   It's just the way it is has always been.  Please don't change libffi.  I'll document this clearly for the next version (perhaps there is a mention of this, I haven't looked yet).

The same is true for returning 8-bit values, for instance, on 32-bit systems.  All ABIs extend those results to the full 32-bits so you need to provide a properly aligned buffer that's big enough to hold the result.

BTW - you can just use ffi_arg as the storage for all integral return values.  Look at the libffi testsuite code, like return_sc.c.  For ppc64 ffi_arg is an unsigned long.

That doesn't sound right to me and totally defeats the point of typing the
return value....

If I tell libffi that I want a return value of type "int", why should I pass a
buffer to something other than "int" ?

What you say means that the caller of libffi would have to hold ABI specific
knowledge, which completely defeats the purpose of libffi here.

So I don't think you are on the right track :-) libffi should convert the
return value to whatever type has been requested by the caller. The caller
shouldn't have to make various assumptions on what ABI it's running on.
  
Now I'm not familiar with libffi, just looking at it in the context of that bug, but what happens for arguments (rather than return values) ?

If I pass an argument of type "int", am I expected to pass a pointer to a 32-bit int or a 64-bit sign extended int ?

My understanding is that the former is true, and that's how glib uses libffi. I don't see why the return argument should work differently.

Note: If that's one of those too-late-to-fix issue (since if we change it, then things using ffi_arg in turn will be broken), then we should probably go through a big fat code audit of everything that uses the library.

IE. I can perfectly see about everybody using it getting it wrong (simply becaues it just doesn't make any sense to do it that way imho :-) not just glib here.

If you don't want to break libffi ABI but still handle return type in a platform agnostic way, I'd suggest adding some function like

ffi_collect_return (ffi_cif *cif, ffi_type rtype, void *rvalue_in, void *rvalue_out);

or something like that.  Which would certainly help insulate callers from platform differences like this.  Then something like glib could avoid return value handling errors with

  if (ffi_prep_cif (&cif, FFI_DEFAULT_ABI, n_args, rtype, atypes) != FFI_OK)
    return;

  ffi_call (&cif, marshal_data ? marshal_data : cc->callback, rvalue, args);

  ffi_collect_return (&cif, rtype, rvalue, rvalue_sanitized);

  if (return_gvalue && G_VALUE_TYPE (return_gvalue))
    value_from_ffi_type (return_gvalue, rvalue_sanitized);

(In reply to comment #39)
> If you don't want to break libffi ABI but still handle return type in a
> platform agnostic way, I'd suggest adding some function like
> 
> ffi_collect_return (ffi_cif *cif, ffi_type rtype, void *rvalue_in, void
> *rvalue_out);

An interesting idea, although I'm not sure it's necessary.  There's nothing platform specific about casting the ffi_arg result to the shorter type that you want, although it's usually not even necessary to do that.

Anyhow, looks like we have to use ffi_arg, it's unfixable, too many things already using it that way.

On the other hand, quite a few users of libffi around seem to basically pass the pointer to their native type along just like glib does, tho it's hard to tell with a simple google code search. You may want to double check the various JNIs etc... out there.

This is such a horribly fragile design decision, so error prone, it's really sad. This is the kind of stuff that people -will- get wrong over and over again and will lead to subtle memory corruption bugs (since the problem is a lot less visible on LE, at least on BE we see the bad return value immediately).

If you meant to always get an ffi_arg for scalar type, then you shouldn't have made the return argument a void * but some kind of union or something a tad more strongly typed.

What made you ever think that doing return values differently than arguments was ever a good idea ?

(In reply to comment #40)
> (In reply to comment #39)
> > If you don't want to break libffi ABI but still handle return type in a
> > platform agnostic way, I'd suggest adding some function like
> > 
> > ffi_collect_return (ffi_cif *cif, ffi_type rtype, void *rvalue_in, void
> > *rvalue_out);
> 
> An interesting idea, although I'm not sure it's necessary.  There's nothing
> platform specific about casting the ffi_arg result to the shorter type that you
> want, although it's usually not even necessary to do that.

For sake of argument:

int foo;
void *rvalue;

rtype = &ffi_type_sint;
rvalue = alloca (ffi_type_sint.size);
ffi_prep_cif (&cif, FFI_DEFAULT_ABI, n_args, rtype, ...);
ffi_call (&cif, callback, rvalue, ...);

Well, the problem is on ppc64 (and maybe any 64-bit BE platform) that casting directly doesn't work.  To get the right value out of an ffi_type_sint return value, you need to:

foo = (int) (*(long*) rvalue);

instead of what you'd expect, which would be:

int foo = (int) rvalue;

since on ppc64 the second one (the direct cast) returns the wrong 32 bits of the 64-bit value from ffi_call().  Or am I missing something here?

(In reply to comment #42)
> 
> (In reply to comment #40)
> > (In reply to comment #39)
> > > If you don't want to break libffi ABI but still handle return type in a
> > > platform agnostic way, I'd suggest adding some function like
> > > 
> > > ffi_collect_return (ffi_cif *cif, ffi_type rtype, void *rvalue_in, void
> > > *rvalue_out);
> > 
> > An interesting idea, although I'm not sure it's necessary.  There's nothing
> > platform specific about casting the ffi_arg result to the shorter type that you
> > want, although it's usually not even necessary to do that.
> 
> For sake of argument:
> 
> int foo;
> void *rvalue;
> 
> rtype = &ffi_type_sint;
> rvalue = alloca (ffi_type_sint.size);
> ffi_prep_cif (&cif, FFI_DEFAULT_ABI, n_args, rtype, ...);
> ffi_call (&cif, callback, rvalue, ...);
> 
> Well, the problem is on ppc64 (and maybe any 64-bit BE platform) that casting
> directly doesn't work.  To get the right value out of an ffi_type_sint return
> value, you need to:
> 
> foo = (int) (*(long*) rvalue);
> 
> instead of what you'd expect, which would be:
> 
> int foo = (int) rvalue;
> 
> since on ppc64 the second one (the direct cast) returns the wrong 32 bits of
> the 64-bit value from ffi_call().  Or am I missing something here?

so benh says using ffi_arg for rvalue instead of void would do the trick and then we could directly cast.  So nevermind this.

So Anthony, correct me if I'm wrong, but what that means is that
anything that's "bridging" two types interface, ie glib vs. ffi
java vs. ffi, js vs. ffi, smalltalk vs. ffi (to take a few examples
I picked with google) must do something along those lines:

 Is return type scalar ?

    Yes : use a pointer to a temp ffi_arg
    No  : use a pointer to the original return type

 Do the call

 Was return type scalar ?

    Yes : Do some kind of giant switch/case that appropriately casts
          that ffi_arg to the appropriate original type

From what I can tell, the various bridges i've seen around don't do that,
so it's VERY possible that I've not totally grasped what they do (they are
all quite convoluted code and all seem to manipulate language implementation
specific types that I don't know about) but it looks like they also all
do something akin to what glib did (which is broken) which is to essentially
pass a pointer to the original type to ffi_call and concert the type itself
to the appropriate ffi_type, just like for arguments.

If I'm right, that means that the majority of your users out there are wrong
and happily silently corrupting memory beyond the return value on x86_64
(or getting the return value wrong on ppc64/sparc64/mips64).

I would VERY STRONGLY suggest that you revisit this and provide a new variant
of ffi_call that converts the return values so that the pointer passed by the caller corresponds precisely to the type requested, just like arguments, and then start the process of deprecating the current variant of ffi_call.

The current situation as far as I can tell is just way too error prone and boils down to bad engineering.

Dan: Won't work on x86, double won't fit 64-bit, unless ffi_arg is big enough to hold it there.

I'm afraid you'll really have to special case scalar vs. non-scalar.

I don't have the bandwidth for that, but somebody should really audit all users of libffi, that stuff smells real bad.

(In reply to comment #41)
> On the other hand, quite a few users of libffi around seem to basically pass
> the pointer to their native type along just like glib does, tho it's hard to
> tell with a simple google code search. You may want to double check the various
> JNIs etc... out there.

I'll write something up for the libffi-discuss list.

> If you meant to always get an ffi_arg for scalar type, then you shouldn't have
> made the return argument a void * but some kind of union or something a tad
> more strongly typed.
> 
> What made you ever think that doing return values differently than arguments
> was ever a good idea ?

I don't recall.  It was 15 years ago.

Fair enough, I did stupid stuff 15 years ago too :-) Still, I believe it would be useful to introduce a new variant of ffi_call that does argument conversion.

In the end, the callers will have to do it if libffi doesn't do it and I'd rather see that sort of stuff done right in one place than with 5 different bugs in 5 different places.

(In reply to comment #44)
> So Anthony, correct me if I'm wrong, but what that means is that
> anything that's "bridging" two types interface, ie glib vs. ffi
> java vs. ffi, js vs. ffi, smalltalk vs. ffi (to take a few examples
> I picked with google) must do something along those lines:
> 
>  Is return type scalar ?
> 
>     Yes : use a pointer to a temp ffi_arg
>     No  : use a pointer to the original return type
> 
>  Do the call
> 
>  Was return type scalar ?
> 
>     Yes : Do some kind of giant switch/case that appropriately casts
>           that ffi_arg to the appropriate original type

Essentially, yes.

> From what I can tell, the various bridges i've seen around don't do that,

I just checked python (ctypes), JNA, Ruby FFI and gcj and they all get it
right.

This would be an interesting check for one of the static analysis tools out
there to add.

> I would VERY STRONGLY suggest that you revisit this and provide a new variant
> of ffi_call that converts the return values so that the pointer passed by the
> caller corresponds precisely to the type requested, just like arguments, and
> then start the process of deprecating the current variant of ffi_call.

Fair enough.  This may be of value, since many users have to go through this
manual step.  As I mentioned, I'll send a note to libffi-discuss.  Patches are
also welcome, of course.

Some of the reasoning for this decision is coming back to me.  The thought was
that some users will immediately want to promote the result back, so leaving
those results in their promoted sizes would be a win in those cases.  Or
something like that.  15 years is a long time!

> Some of the reasoning for this decision is coming back to me.  The thought was
> that some users will immediately want to promote the result back, so leaving
> those results in their promoted sizes would be a win in those cases.  Or
> something like that.  15 years is a long time!

But such users could just have requested a long and be done with it :-)

No biggie, I'm happy you managed to parse JNA code, I gave up :-)

One I haven't managed to get my head around is

http://code.google.com/p/gpsee/source/browse/modules/gffi/CFunction.c?spec=svn312dc94085ad58a411b4b04281f9dbe4667e6b41&r=312dc94085ad58a411b4b04281f9dbe4667e6b41

There was another one (smalltalk realted) but can't seem to find it anymore,
and of course glib :-)

I think Davlik (android) has it wrong. I didn't find the canonical repo from google but there's a few offshoots here or there, here's one of them:

http://gitorious.org/0xdroid/dalvik/blobs/master/vm/arch/generic/Call.c

The uint32 case will be wrong here as far as I can tell

I will be away from friday for a few weeks, I don't have time to help much
more with this, sorry about that, hopefully Dan will be able to sort out glib
and we'll see what else eventually hits

Phil and I were looking at this today and the various patches didn't seem to allow glib2 to pass tests.  Are we still contemplating our next move?

Previous comment was by Brent (firefox seemed to hold my bugproxy-ness).  This bug(s) is keeping us from alpha on ppc64.

Right, Dan needs to do a new patch for the return values, are there other tests that fail elsewhere ?

Quick update from Karsten via phone today:

He managed to compose new images yesterday with Ben's fix in libffi and the ones that Dan mentioned need to be in glib (back out one of the "wrong" patches).

The second marshaller test still fails now, but the installer now successfully works with NM and NM doesn't need to be manually restarted in the installer anymore!

IIRC Dan also mentioned he tested the libffi change on x86_64 if it breaks things there but at least according to his tests they didn't.

In any case, please retest with the latest images available from:

http://ppc.koji.fedoraproject.org/scratch/karsten/iso/Fedora-20110928-ppc64-DVD.iso

If those work we'll use the fixes for now, at least for PPC64.

Thanks & regards, Phil

Unfortunately I need to get back to the networking stuff I've been ignoring the last few days trying to track this down, so we need to get the glib people to work on this instead, since glib is where the bug actually is (well, and libffi's odd API but whatever).  So either Matthias or Colin Walters need to take this bug over at this point, now that we've diagnosed what the issue is.  I'm happy to grab and test whatever fixes they come up with on power02.str since that wouldn't take a large amount of time but I can't spend a ton of time on this bug for a bit...

My recommendation is to back out the libffi patch since it's not sanctioned upstream, and apply the attached patch to glib2 builds, pending approval by glib maintainers.

RFC/proposed glib patch to address marshalling issues:

http://bugzilla-attachments.gnome.org/attachment.cgi?id=197698

It was my understanding that the glib2 patch alone was not enough to make NM work.  Do others recall this as well?

I would recommend renaming value_to_ffi_type to rvalue_to_ffi_type : added the "r" to make it clear that this function is only to be used on a return value, maybe add a comment explaining why we do that too, ie:

 /* ffi_call return value for scalars is always an ffi_arg, regardless
  * of the requested type, so we need to convert it appropriately

Brent: I think the previous glib patch wasn't enough as it didn't handle the return value problem, the new patch should make it work.

Do we still have failures in glib2 tests ? Is there another case that isn't handled properly ?

(In reply to comment #57)
> It was my understanding that the glib2 patch alone was not enough to make NM
> work.  Do others recall this as well?

No, the glib2 patch alone should make NM work.  The original problem was failure of glib to properly marshal enums which NM was depending for inotify events, which would allow NM to detect that anaconda had updated the ifcfg file.

(In reply to comment #59)
> Brent: I think the previous glib patch wasn't enough as it didn't handle the
> return value problem, the new patch should make it work.
> 
> Do we still have failures in glib2 tests ? Is there another case that isn't
> handled properly ?

The linked patch passes the marshalling tests that we added to glib to expose this bug.  AFAIK there is no other case that we care about that fails.  We do need better testcase coverage of all the other return types, but given the changes in this patch those *should* work (but wouldn't hurt to verify).

The patch has been committed to upstream glib.

glib2-2.30.0-2.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/glib2-2.30.0-2.fc16

Package glib2-2.30.0-2.fc16:
* should fix your issue,
* was pushed to the Fedora 16 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing glib2-2.30.0-2.fc16'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-2011-13892
then log in and leave karma (feedback).

glib2-2.30.0-2.fc16 has been pushed to the Fedora 16 stable repository.  If problems still persist, please make note of it in this bug report.

If you ever want to try and get your head around the gffi module mentioned in comment 49, please let me know. Sorry about the readability. I expect it to be relatively future-safe and portable across SUS v3 (and Linux) providing the libffi API does not change.  It was developed on 3.0.8 and we currently use 3.0.10.  Tested on x86_64, x86, 32-bit sparc v9.  Mac, Linux, Solaris.

I wrote that code several years ago, but I do recall having to handle type marshalling differently for return values and arguments, and being puzzled by that.

IIRC, the marshalling converts between JavaScript values and appropriate C types, then converts between the C types and the FFI types.  ISTR int-like values are categorized by examining the storage size and signedness.

The type converters (marshalling functions) are decided when the JS CFunction class is instanciated, based on the function arguments passed in the constructor. The type converter which is actually invoked for a given argument is stored as a function pointer in the argConverters array, which is unique per instance of the JS CFunction class.