After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 695279 - GParted doesn't compile on RHEL / CentOS 5.9
GParted doesn't compile on RHEL / CentOS 5.9
Status: RESOLVED FIXED
Product: gparted
Classification: Other
Component: application
GIT HEAD
Other Linux
: Normal normal
: ---
Assigned To: gparted maintainers alias
gparted maintainers alias
Depends on:
Blocks:
 
 
Reported: 2013-03-06 11:18 UTC by Mike Fleetwood
Modified: 2013-04-30 16:30 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
RHEL/CentOS 5 build fixes (v1) (31.85 KB, patch)
2013-03-10 22:09 UTC, Mike Fleetwood
none Details | Review
RHEL/CentOS 5 build fixes (v2) (31.86 KB, patch)
2013-03-20 09:39 UTC, Mike Fleetwood
none Details | Review
Debug log #1 (7.69 KB, text/plain)
2013-04-23 19:01 UTC, Mike Fleetwood
  Details
RHEL5 build extras (draft 1 (4.53 KB, patch)
2013-04-23 19:10 UTC, Mike Fleetwood
none Details | Review
Less threading patch (draft 1) (1.11 KB, patch)
2013-04-23 19:42 UTC, Mike Fleetwood
none Details | Review
RHEL5 build extras (v1) (10.32 KB, patch)
2013-04-27 09:46 UTC, Mike Fleetwood
none Details | Review
RHEL5 build extra (v2) (12.27 KB, patch)
2013-04-28 13:06 UTC, Mike Fleetwood
none Details | Review
Patch to fix change in default behaviour introduced with gcc 4.6 (2.34 KB, patch)
2013-04-29 19:58 UTC, Curtis Gedak
none Details | Review
Fix set_default_icon_name autoconf check (v1) (1.86 KB, patch)
2013-04-29 21:31 UTC, Mike Fleetwood
none Details | Review

Description Mike Fleetwood 2013-03-06 11:18:59 UTC
There are 2 issue with compiling current GParted:

1) Utils::regexp_label() uses Glib::Regex class, but it's not available.
   RHEL / CentOS 5.9 comes with Glibmm 2.12, but the Glib::Regex class
   is new in Glibmm 2.14;
https://developer.gnome.org/glibmm/2.34/classGlib_1_1Regex.html#details

2) Win_GParted::Win_GParted() calls Gtk::Window::set_default_icon_name()
   member function.  This is reported available in Gtkmm >= 2.6 and
   RHEL / CentOS 5.9 comes with gtkmm 2.10.  However the method is not
   actually available on RHEL / CentOS 5.9.
http://developer.gnome.org/gtkmm/3.6/classGtk_1_1Window.html#a533d03e9b92d8ccd142ab3a44005cae4

Will fix by adding suitable autoconf feature tests for these.
Comment 1 Mike Fleetwood 2013-03-10 22:09:46 UTC
Created attachment 238546 [details] [review]
RHEL/CentOS 5 build fixes (v1)

Hi Curtis,

Here's the patches to make GParted build on RHEL/CentOS 5.9.

I've tested them successfully on Fedora 14 & 17, CentOS 5.9 & 6.3 and
Debian 6.

Thanks,
Mike
Comment 2 Mike Fleetwood 2013-03-10 23:00:11 UTC
Oh,  Also available from:
    https://rockover.homeip.net/cgit/gparted/
    Branch: rhel5-build-v1
Comment 3 Mike Fleetwood 2013-03-18 11:42:44 UTC
Hi Curtis,

Just wanted to mention that from what I can tell the normal / best
practice for doing autoconf checks for HAVE_METHOD is to search in the
library or compile or link a test program to test for METHOD.  Given
that C++ mangles function names searching libraries for method names
doesn't work so you have to do compile or link tests.  Therefore using
checks for versions of components to determine the availability of
METHOD is not the ideal way.

Specifically the test for HAVE_SET_DEFAULT_ICON_NAME follows autoconf
best practice by performing a compile and link test.  This had to be
done for the reasons noted in the patch.  The tests for HAVE_GLIB_REGEX,
HAVE_GTK_SHOW_URI and HAVE_GET_MESSAGE_AREA use the non-ideal way as
they use pkg to check the version of a library install and assume the
method is available from that.

I had a look at gnome-system-monitor's configure.ac, it's a C++ app too,
and it makes use of pkg checks.
    http://git.gnome.org/browse/gnome-system-monitor/tree/configure.ac

So what I am saying is that I think it's OK to use pkg version checks so
long as we understand that it is a short cut for a more accurate but a
lot slower, especially for C++ and GNOME libs, compile and link checks.

We have to stick with link and run checks for the version of libparted
because while recent distributions provide pkg information for
libparted, RHEL/CentOS 5.9 and possibly other old distributions don't.

Mike
Comment 4 Mike Fleetwood 2013-03-20 09:39:19 UTC
Created attachment 239337 [details] [review]
RHEL/CentOS 5 build fixes (v2)

Hi Curtis,

Rebased this patch to apply cleanly on top of the current master,
commit:
5b737b224de9f868ecf91a54994b35b0c78abebc

The only change was to update the first line of the file configure.ac
with:
    AC_INIT([gparted],[0.15.0-git],...

Thanks,
Mike
Comment 5 Curtis Gedak 2013-03-20 19:32:29 UTC
Hi Mike,

Thank you for all your work to enable GParted to compile on even more distribution versions.  :-)

I am working on the GParted 0.15.0 release for the GParted Live portion, and have quite the backlog of patches to review, so please bear with me if it takes me a while to get to all of your improvements.

I agree with your reasoning in comment #3.  In some circumstances we will have to use a test compile to verify a feature.  Otherwise I am okay with a simple package check.

For this report I confirmed that GParted compiled and ran on the following distros:

    Debian 6
    Ubuntu 10.04
   Kubuntu 12.04
    Fedora 16
  openSUSE 11.2

The patch set in comment #4 has been committed to the git repository for inclusion in the next release of GParted.

The relevant git commits can be viewed at the following links:

Implement fallback if Glib::Regex class is missing (#695279)
https://git.gnome.org/browse/gparted/commit/?id=456932846bfbfbd77bbf49165b9eb6c2b84e0da6

Only use Gtk::Window::set_default_icon_name method when available (#695279)
https://git.gnome.org/browse/gparted/commit/?id=a04210788399736ff7f097cb75650ebcbd0a4950

Modernise to a minimum of autoconf 2.50
https://git.gnome.org/browse/gparted/commit/?id=40fb05f043b7ed7ad315bad192af2e37c5e8498e

Add autoconf checking messages for two checks
https://git.gnome.org/browse/gparted/commit/?id=12d2cad682f1da06e2c59459ba73d2715cb1e615
Comment 6 Mike Fleetwood 2013-04-17 11:39:56 UTC
Hi Curtis,

I have found that additional fixes are now needed to allow GParted to
compile of RHEL / CentOS 5.9.

    (When I rebased the code in comment #4 to account for GParted 0.15.0
    I must have only re-tested it on my Fedora 14 desktop and not CentOS
    5.9 too :-(


Patch to allow GParted to compile and run on RHEL / CentOS 5.9 can be
seen here:
    http://rockover.homeip.net/cgit/gparted/log/?h=rhel5-devel3

I've not submitted it yet because I'm still looking into the following
runtime warning with GParted 0.15.0, probably everytime a thread is
being created:

    # ~centos/programming/c/gparted/src/gpartedbin
    ======================
    libparted : 1.8.1
    ======================
    Failed to change to directory '' (No such file or directory)
    Failed to change to directory '' (No such file or directory)
    Failed to change to directory '' (No such file or directory)
    Failed to change to directory '' (No such file or directory)
    ...


So for GParted 0.16.0 we can either:

1)  Go with what is already in main line but not be able to say this bug
    is fixed:
        Bug 695279 - GParted doesn't compile on RHEL / CentOS 5.9

2)  Take the patch I have written so far and be able to say GParted can
    compile on RHEL / CentOS 5.9:
        Further RHEL / CentOS 5.9 compile fixes (#695279)

3)  Wait until I fix the "Failed to change to directory" warning too.

Probably option (2) is best.  Left me know and I'll submit the patch if
you want.

Thanks,
Mike
Comment 7 Mike Fleetwood 2013-04-17 11:55:00 UTC
Drat.  The "Failed to change to directory" warning issue is also
stopping all external commands from being executed.  Therefore GParted
can't read any unmounted file system usage, create any file system, etc. 
 
Option (2) above has become a non-started.  Having GParted run but not
be able to execute and commands is worse than not compiling.

Perhaps option (1) is the right choice.
Comment 8 Curtis Gedak 2013-04-17 16:37:55 UTC
Hi Mike,

We can go with option (1) if need be.  If you have a fix before we release 0.16.0, then we can include it as well.

Another option we have available to us is to create a git branch at GPARTED_0_15_0, and only apply the necessary patch for fixing bug 697727 - segfault in gparted copy.  My preference is to not do this because the branch will clutter the git repository.
Comment 9 Mike Fleetwood 2013-04-18 19:11:02 UTC
Progress update.


Fixed the problem with not executing commands and getting error "Failed
to change to directory '' ...".  See patch here:
    http://rockover.homeip.net/cgit/gparted/log/?h=rhel5-devel3


I'm now getting GParted reporting the following error 3 times and the
occasional core dump, but only on CentOS 5.9.
(gpartedbin:9670): GLib-CRITICAL **: g_source_unref: assertion `source != NULL' failed

Works correctly on Fedora 14 and Debian 6.  Suspect (another)
gtkmm/glibmm memory corrupting bug.  Top of the backtrace:

  • #0 _int_malloc
    from /lib/libc.so.6
  • #1 malloc
    from /lib/libc.so.6
  • #2 IA__g_malloc
    at gmem.c line 131
  • #3 miRegionOp
    at gdkregion-generic.c line 845
  • #4 IA__gdk_region_intersect
    at gdkregion-generic.c line 581
  • #5 gdk_window_process_updates_internal
    at gdkwindow.c line 2308
  • #6 IA__gdk_window_process_all_updates
    at gdkwindow.c line 2387
  • #7 gdk_window_update_idle
    at gdkwindow.c line 2245
  • #8 g_idle_dispatch
    at gmain.c line 3926
  • #9 g_main_dispatch
    at gmain.c line 2045
  • #10 IA__g_main_context_dispatch
    at gmain.c line 2596
  • #11 g_main_context_iterate
    at gmain.c line 2677
  • #12 IA__g_main_loop_run
    at gmain.c line 2881
  • #13 IA__gtk_main
    at gtkmain.c line 1001
  • #14 Gtk::Main::run_impl()
    from /usr/lib/libgtkmm-2.4.so.1
  • #15 Gtk::Main::run()
    from /usr/lib/libgtkmm-2.4.so.1
  • #16 GParted::GParted_Core::set_devices
    at GParted_Core.cc line 155
  • #17 GParted::Win_GParted::menu_gparted_refresh_devices
    at Win_GParted.cc line 1252
  • #18 Gtk::Widget_Class::show_callback(_GtkWidget*)
    from /usr/lib/libgtkmm-2.4.so.1

Comment 10 Curtis Gedak 2013-04-18 19:47:36 UTC
Mike, we found a thread wait problem in bug #697518 - gparted scans forever blank disks in virtual machine.  Perhaps try the patch from there?
Comment 11 Mike Fleetwood 2013-04-23 14:49:46 UTC
Tried using GParted master 6e2de3b Updated Romanian Translation which
include fix for bug 697518 but it didn't help.


Using valgrind I has seen:

1) Multiple ioctl() syscalls using uninitialised bytes in
   ped_device_open/close/sync.

2) Multiple write() syscalls using uninitialised bytes which I think are
   cairo rendering sending data to X11 to draw.

Not sure that (2) is relevant.  Also using valgrind makes the assertion
error go away.

(gpartedbin:9670): GLib-CRITICAL **: g_source_unref: assertion `source != NULL' failed


Used printing to stdout to track the errors as being produced by this
call to Gtk::Main::run() on line 153 in GParted_Core::set_devices().

   147  void GParted_Core::set_devices( std::vector<Device> & devices )
   148  {
   149          Glib::Thread::create( sigc::bind(
   150                                  sigc::mem_fun( *this, &GParted_Core::set_devices_thread ),
   151                                  &devices),
   152                                false );
>> 153          Gtk::Main::run();
   154  }
Comment 12 Curtis Gedak 2013-04-23 17:53:54 UTC
Hi Mike,

In order to see this problem I had to go back to OpenSUSE 11.2, as the
problem does not occur in many of my VMs, including OpenSUSE 12.2.

With the latest GParted master 6e2de3b Updated Romanian Translation
code, I see the error 5 times on GParted startup in OpenSUSE 11.2:

(gpartdbin:8284): GLib-CRITICAL **: g_source_unref: assertion `source
!= NULL' failed

This VM has two disks, sda has linux-swap and ext4 partitions, sdb has
ext4 partition.


Since I ran the same gparted code in both OpenSUSE 11.2 and 12.2, I
too suspected that there was some gtkmm/glibmm bug in these earlier
distros.

Following this line of thinking I tested some of the older Ubuntu
distros as well.


Distro          Error?    glibmm2 version      gtkmm2 version
-------------   ------   -----------------   -----------------
OpenSUSE 11.2     Yes      2.22.1-2.2          2.18.2-2.5
OpenSUSE 12.2      No      2.32.0-2.1.2        2.24.2-5.1.2

Ubuntu  8.10       No*     2.18.1-1          1:2.14.1-0ubuntu1
Ubuntu  9.10       No      2.22.1-2          1:2.18.2-1
Ubuntu 10.04       No      2.24.2-0ubuntu1   1:2.20.3-0ubuntu1


* A different error is seen on Ubuntu 8.10.  Specifically:

Failed to change to directory '' (No such file or directory)


While performing tests with Ubuntu, I had expected the g_source_unref
error to occur with Ubuntu 9.10 since this used similar versions of
glibmm and gtkmm as OpenSUSE 11.2.  What I discovered is that even
Ubuntu 8.10 with earlier versions of glibmm and gtkmm did not exhibit
the g_source_unref problem.

This leads me to believe that there is some other piece of the puzzle
that leads to the g_source_unref behaviour.
Comment 13 Mike Fleetwood 2013-04-23 19:01:02 UTC
Created attachment 242282 [details]
Debug log #1

I've tracked the assert errors further to here:

    13  void PipeCapture::connect_signal( int fd )
    14  {
    15          // connect handler to signal input/output
    16          connection = Glib::signal_io().connect(
    17                  sigc::mem_fun( *this, &PipeCapture::OnReadable ),
    18                  fd,
    19                  Glib::IO_IN | Glib::IO_HUP | Glib::IO_ERR );
    20  }

It only occurs when GParted is executing 3 of the external commands of
the many it does during refresh.

Selected lines of output from the debug messages I've added to gparted
investigating this fault.  Full log attached.

D: GParted_Core::read_label(partition) partition.get_path()="/dev/sda1"
D: ext2::read_label()
D: Utils::execute_command(command="e2label /dev/sda1", output, error, use_C_locale=1)
D: Utils::execute_command() status.foreground=0
D: Utils::execute_command() Calling outputcapture.connect_signal(out) ...
D: PipeCapture::connect_signal(fd=10)

(gpartedbin-orig:2279): GLib-CRITICAL **: g_source_unref: assertion `source != NULL' failed
D: PipeCapture::connect_signal() return
D: Utils::execute_command() Calling outputcapture.connect_signal(err) ...
D: PipeCapture::connect_signal(fd=12)
D: PipeCapture::connect_signal() return
D: Utils::execute_command() return 0  (status.exit_status)
D: return ext2::read_label()
D: return GParted_Core::read_label()


The problem is I don't know what this assertion error is really means.
I have been assuming it meant some malloc/free heap corruption, but
valgrind suggests not.  So it could be just another type of misuse of
some glibmm tracked object.
Comment 14 Mike Fleetwood 2013-04-23 19:10:08 UTC
Created attachment 242285 [details] [review]
RHEL5 build extras (draft 1
Comment 15 Mike Fleetwood 2013-04-23 19:11:33 UTC
[To go with file added in comment #14 above]

These 2 patches make GParted compile on CentOS 5.9 but they don't solve
the assertion errors.  I am posting them for information and
investigation purposes.  I think that they shouldn't be applied to
mainline until we resolve the assertion errors.  I think that it is
better to have GParted not compile rather than compile but have runtime
crash potential.  Hence the description of draft patches.
Comment 16 Mike Fleetwood 2013-04-23 19:42:50 UTC
Created attachment 242287 [details] [review]
Less threading patch (draft 1)

I can fix the fault by applying the attached patch.  I reduces the
threading in GParted and reverses a small bit of this change:

    Reduce threading (#685740)
    https://git.gnome.org/browse/gparted/commit/?id=52a2a9b00a32996921ace055e71d0e09fb33c5fe

The main thread in GParted creates a separate Glib::Thread thread to run
GParted_Core::set_devices_thread() presumably to allow the main thread
to respond to UI requests while set_devices_thread() scans the devices
for partitions and the file system information include running lots of
executables via Utils::execute_command() using
Glib::spawn_async_with_pipes().

My change removes the separate thread to run set_devices_thread().  As
all the widgets in the UI are set to insensitive and grayed for the
duration of the refresh I don't think there is any loss of
functionality.  However I don't know very much about glib(mm) threading
etc so I don't know why this fixes it and whether it's a good idea or
not.

Perhaps Phillip can have a look at this as he seems to understand the
glib(mm) threading stuff.
Comment 17 Curtis Gedak 2013-04-24 17:34:10 UTC
Hi Mike,

The set_devices() function was threaded even before I joined the GParted project back in 2007.  Previously the thread was set up in Win_GParted.cc.  Following is a link to how this was done in GParted 0.3.7:

https://git.gnome.org/browse/gparted/tree/src/Win_GParted.cc?id=GPARTED_0_3_7#n974

The thread enables updates to the GUI of the scanning / searching progress.  These updates help our users and us to figure out what is going on.  This was most recently helpful in triaging bug 697518 - gparted scans forever blank disk in virtualbox.  Specifically we knew the thread was hanging when "Searching /dev/sdb partitions", where /dev/sdb was a blank disk with no partition table.


In my testing with OpenSUSE 11.2, I have noticed that the g_source_unref problem does not consistently appear.  This leads me to believe that there is some sort of timing issue between the threads that varies from run to run.

I think it would be best if we can come up with a solution that does not involve removing the thread for set_devices().


Phillip,

Any ideas?
Comment 18 Curtis Gedak 2013-04-24 18:34:32 UTC
Hi Mike,

All of your investigation details led me to look at how the threads are working.

From a quick comparison of the threading in Utils.cc and FileSystem.cc, one tracks foreground status and uses mutexes, and the other does not.

Utils::execute_command()
https://git.gnome.org/browse/gparted/tree/src/Utils.cc?id=GPARTED_0_16_0#n462

FileSystem::execute_command()
https://git.gnome.org/browse/gparted/tree/src/FileSystem.cc?id=GPARTED_0_16_0#n80

If I recall correctly the main difference between these two implementations is that one (FileSystem) updates the OperationDetail entries, whereas the other (Utils) does not.

I think we might need this extra tracking and mutex handling in FileSystem::execute_command().

I'm going to try to add this to FileSystem::execute_command() to see what happens.
Comment 19 Phillip Susi 2013-04-24 20:23:25 UTC
The FileSystem::execute_command() doesn't need a mutex because it is never called outside the main thread.  It looks like this is bug #697727.  I fixed it by converting all of the calls to the glibmm GSource wrappers to use glib directly since the glibmm wrappers are broken.  It looks like I missed that one in PipeCapture using signal_io.
Comment 20 Mike Fleetwood 2013-04-25 08:16:28 UTC
Phillip,

Can you produce a patch please.


Curtis,

Will we need to release GParted 0.16.1 and hold off releasing GParted
Live CD until then?
Comment 21 Curtis Gedak 2013-04-25 14:39:35 UTC
(In reply to comment #20)
> Will we need to release GParted 0.16.1 and hold off releasing GParted
> Live CD until then?

I think it would be prudent to keep GParted Live 0.16.0 in the testing branch and not promote it to stable.  In the meantime we can continue work to address this issue and then release 0.16.1.

In fact I was thinking of this last night.  I came up with some theories of potential problems and need to investigate further to see if my theories have merit.
Comment 22 Curtis Gedak 2013-04-25 15:53:50 UTC
To keep our users up-to-date, I have added a caution regarding a possible thread timing issue in GParted 0.16.0 to the GParted website download and news pages.  I have also added the caution to the SF gparted-0.16.0 README and reverted the stable link to 0.14.1 on the GParted web site front page.
Comment 23 Curtis Gedak 2013-04-25 20:37:13 UTC
Hi Mike and Phillip,

Please let me know if the following hypothesis and logic is flawed.


HYPOTHESIS
----------

When spawning commands asynchronously, We incorrectly assume the
parent thread is always the main thread.


BACKGROUND
----------

In Utils::execute_command, we use a mutex lock/unlock to ensure the
parent thread finishes output and error stream capture setup before
the child thread proceeds.  However the code only sets up a mutex
lock/unlock if the parent is the *main* thread or "foreground" thread.
See,

https://git.gnome.org/browse/gparted/tree/src/Utils.cc?id=GPARTED_0_16_0#n474

From reading the code, I believe there are situations where
Utils::execute_command is called outside of the main thread.


EXAMPLE OF Utils::execute_command CALL OUTSIDE MAIN THREAD
----------------------------------------------------------

1)  The main thread is the one started in main.cc that runs GParted.

https://git.gnome.org/browse/gparted/tree/src/main.cc?id=GPARTED_0_16_0#n28


2)  This main thread creates class Win_GParted which in turn creates
    class GParted_Core.

https://git.gnome.org/browse/gparted/tree/src/main.cc?id=GPARTED_0_16_0#n55


3a)  When the Win_GParted class method menu_gparted_refresh_devices() calls:

  gparted_core .set_devices( devices );

https://git.gnome.org/browse/gparted/tree/src/Win_GParted.cc?id=GPARTED_0_16_0#n1253

3b)  the GParted_Core class creates a second thread for:

  GParted:Core::set_devices_thread

https://git.gnome.org/browse/gparted/tree/src/GParted_Core.cc?id=GPARTED_0_16_0#n147

  Note:  This second thread is *not* the main thread.


4)  In this second thread, the GParted_Core::set_devices_thread method
    refreshes several caches of information.  E.g., FS_Info.

https://git.gnome.org/browse/gparted/tree/src/GParted_Core.cc?id=GPARTED_0_16_0#n162


5)  Inside FS_Info, the load_fs_info_cache method is called:

https://git.gnome.org/browse/gparted/tree/src/FS_Info.cc?id=GPARTED_0_16_0#n59

6)  The load_fs_info_cache method calls Utils::execute_command
    from this second thread.

https://git.gnome.org/browse/gparted/tree/src/FS_Info.cc?id=GPARTED_0_16_0#n64


Hence, calls to Utils::execute_command are not always from the main
thread.

This call to Utils::execute_command will not benefit from mutex lock
protection because it is not the main thread.


THREAD DIAGRAM
--------------

Following is an attempt to illustrate the call of
Utils::execute_command from a thread other than the main thread.

  THREAD 1       THREAD 2           THREAD 3
  --------       ------------       --------

   main.cc
      main
    thread

    Parent   --> Child

                 GParted_Core
                  set_devices
                       thread

                       Parent   --> Child

                                    FS_Info
                                    blkid command execution


SUGGESTED APPROACH FOR A SOLUTION
---------------------------------

We need to find some other method to identify the immediate
relationship between parent and child.  The current method of
identifying the main thread, or root parent only works if there are at
most two threads.


FURTHER IMPLICATION
-------------------

If the above reasoning is sound, then I believe we will need similar
parent and child mutex race protection for
FileSystem::execute_command.
Comment 24 Phillip Susi 2013-04-25 20:55:28 UTC
As I said before, the Filesystem::execute_command() is always called from the main thread, so didn't need the mutex.  When Utils::execute_command() is called from the main thread, it likewise does not need a mutex.  It only needs it ( and hence only uses it ) when called from a background thread.  When it is called from the foreground thread, it doesn't need a mutex because there is no other thread with which it needs to synchronize.  When called from a background thread, it has to synchronize with the main thread because it is the main thread that gets the IO and child process exited notifications.  The mutex is used for the main thread to signal the background thread that the child is done.  When called in the foreground thread, the Gtk::Main::run() just returns once the child is done.
Comment 25 Curtis Gedak 2013-04-26 01:51:35 UTC
Mike,

Phillip has posted a patch for the g_source_unref problem in:
  bug 697727 comment 46

My initial testing in my OpenSUSE 11.2 VM with this patch is promising.  So far I have not encountered any more of the g_source_unref messages.

Would you be able to test this patch as well?

Thanks,
Curtis
Comment 26 Curtis Gedak 2013-04-26 02:13:35 UTC
Mike,

Following is a snippet from an IRC conversation between Phillip and me on the topic of threads.  

I introduced this topic in comment 18 and comment 23, but it is really off topic for this bug report.  The original posts arose from me confusing spawned command processes which are communicating via pipes, with multiple threads and shared memory communication.

In an effort to close this thread *pun intended* I have added this IRC conclusion:


<gedakc> Question on threads.  With only two threads, is there some magic that automatically occurs so that they can properly communicate?  And this magic does not happen for greater than 2 threads?
<gedakc> As you can tell I'm still unsure of where mutexes are needed.
<psusi> gedakc, mutexes are needed to prevent two threads from running code at the same time that manipulate a set of data where the order of operations matters... i.e. make sure thread A doesn't change data while thread B is looking at it so it sees data that is partially updated
<psusi> in the case of PipeCapture::execute_command, there is only one thread; the main one
<psusi> I would have gotten rid of the threads that you noticed elsewhere that still call Utils::execute_command, but decided it would be too much work... in hindsight that may have been a mistake
<gedakc> I think this is starting to dawn on me.  The spawned commands are PROCESSES, not THREADS.  As such these are communicating via pipes.
<psusi> right
<gedakc> Pipes will naturally wait for the writer or reader.
<psusi> different process, no shared data, so no need for a mutex
<psusi> yea, the kernel takes care of the locking internal to the pipe
<gedakc> I recall you mentioning a mutex for the GParted_Core string thread_status_message.
<gedakc> Since this is referenced in the main thread, but altered in the set_devices_thread, this is a candidate for a mutex.  Correct?
<psusi> yea, iirc, thread_status_message is assigned to in one thread and read in the other with no locking and I don't think ustring does internal locking so it isn't thread safe
<psusi> then again, an operation as simple as assignment just might be atomic so doesn't need locking.. but I haven't checked the ustring internals to see
<gedakc> Hmm... so far it appears to be working, but that might just be luck.
<gedakc> If you don't mind, I would like to paste this thread conversation in the bug report.  Ok?
<psusi> remember, the old code created a background thread to spawn the external process... so that would have needed to synchronize with the main thread... but the whole point of my refactoring was to get rid of that thread and just let the main thread spawn the external processes
<psusi> I don't mind... it seems a bit off topic though since it doesn't actually relate to the bug
<gedakc> I'm thinking of the compile on RHEL bug.  Mostly to close off the digression I created about threads and mutexes.  I think it might help Mike and me.
<psusi> ahh, if you want... that was already digressing from the subject of the bug ;)
<gedakc> I agree.   My bad, but I would like to close off the thread question.
<gedakc> :)
<psusi> ideally I should also refactor the set devices code to not run in a background thread, then Utils::execute_command wouldn't need all that muck to be thread safe
<psusi> that also would have avoided the bugs in glibmm
<gedakc> Always more to do...  ;)  I'm keen to get a stable 0.16.1 release out, but will do more testing before packaging the release.
Comment 27 Mike Fleetwood 2013-04-27 09:46:05 UTC
Created attachment 242648 [details] [review]
RHEL5 build extras (v1)

Hi Curtis,

Here's the finished patch set.  Compared to draft 1 from comment #14
these first 2 patches are the same:

    Further RHEL / CentOS 5.9 compile fixes (#695279)
    Work around failure to execute commands with old glibmm (#695279)

These 2 patches are new:

    Add fallback method for specifying GParted icon (#695279)
    Add copyright notice to PipeCapture .cc and .h files

None of these patches make any translation changes.


Successfully tested on CentOS 5.9 (including build and installing an RPM package).
- This included Phillip's 2nd patch from bug 697727 comment 48.
Successfully tested on Fedora 14 & 17.
Successfully tested and installed on Debian 6.


The only thing that I don't know how to test is how create a release
.tar.bz2 archive from the git tree.  Just wanted to be 100% sure the
changes to data/icons/Makefile.am are OK for the create distribution
archive step.

Thanks,
Mike
Comment 28 Curtis Gedak 2013-04-27 15:25:29 UTC
(In reply to comment #27)
> The only thing that I don't know how to test is how create a release
> .tar.bz2 archive from the git tree.  Just wanted to be 100% sure the
> changes to data/icons/Makefile.am are OK for the create distribution
> archive step.

To create the tarball use:

   make dist

To have the compilation tested and also create the tarball use:

   make distcheck

Hope that helps.
Comment 29 Curtis Gedak 2013-04-27 17:36:47 UTC
Hi Mike,

I have a question on the code in [PATCH 3/4] Add fallback method for specifying GParted icon.

If I am interpreting the code correctly, I think the code will always install a pixmap icon, even for new distros.

Would it be possible to add a check to see if this older pixmap icon is needed?  Then the pixmap icon could conditionally be installed.
Comment 30 Mike Fleetwood 2013-04-28 13:06:22 UTC
Created attachment 242711 [details] [review]
RHEL5 build extra (v2)

Hi Curtis,

This is the same as "RHEL5 build extras (v1)" from comment #27 except
for updating a comment in Makefile.am in existing patch:

    Add fallback method for specifying GParted icon (#695279)

and adding new patch:

    Only install fallback icon when required (#695279)


Successfully tested and installed on CentOS 5.9.

Thanks,
Mike
Comment 31 Curtis Gedak 2013-04-28 20:30:12 UTC
Hi Mike,

Thank you for all your work to add this backward compatibility to GParted.  This should be welcome news for RHEL / CentOS 5.9 users.  :-)

The patches in comment #30 look good to me.  Also my compiles and tests were successful in the following environments: Fedora 13, OpenSUSE 11.2, and Kubuntu 12.04.

As such I have committed the patch in comment #30 to the Gnome git repository for inclusion in the next release of GParted.  This in addition to the commits previously made in comment #5.

The relevant git commits can be viewed at the following links:

Further RHEL / CentOS 5.9 compile fixes (#695279)
https://git.gnome.org/browse/gparted/commit/?id=5f06ea33698791cfd77f1d7607e73bed00d058d0

Work around failure to execute commands with old glibmm (#695279)
https://git.gnome.org/browse/gparted/commit/?id=bae9db222b380b6b7a8feb911b7088c2cbf65b77

Add fallback method for specifying GParted icon (#695279)
https://git.gnome.org/browse/gparted/commit/?id=b09d6035cdca90debb145628b0c62a0213ee1225

Only install fallback icon when required (#695279)
https://git.gnome.org/browse/gparted/commit/?id=d6baac254677b7863af413a38f382e9a2e0252bd
Comment 32 Curtis Gedak 2013-04-28 20:56:35 UTC
Doh!  Just found a minor problem with the patches for this bug report.

When running GParted on Kubuntu 12.04 I see the following:

$ sudo src/gpartedbin
======================
libparted : 2.3
======================
Failed to open file '/usr/local/share/pixmaps/gparted.png': No such file or directory

I think that I did a "make clean" and then "./autogen.sh && make" before this error appeared.

I do not see this problem on Ubuntu 12.04 so perhaps this is related to the K Desktop Environment.
Comment 33 Curtis Gedak 2013-04-28 21:36:05 UTC
It would appear that the check for Gtk::Window::set_default_icon_name method fails on KDE, at least in kubuntu 12.04.

$ ./autogen.sh
<snip>
checking for GTKMM... yes
checking for Glib::Regex class... yes
checking for Gtk::Window::set_default_icon_name method... no
checking for gtk_show_uri function... yes
checking for Gtk::MessageDialog::get_message_area() method... yes
<snip>
Comment 34 Mike Fleetwood 2013-04-29 12:04:48 UTC
(In reply to comment #32)
> $ sudo src/gpartedbin
> Failed to open file '/usr/local/share/pixmaps/gparted.png': No such file or
> directory

The good news is that using the fallback icon works and it only needs
installing with "sudo make install" or minimally with
"(cd data/icons; sudo make install-pixmap)" to make the error go away.


(In reply to comment #33)
> It would appear that the check for Gtk::Window::set_default_icon_name method
> fails on KDE, at least in kubuntu 12.04.

Just installed Kubuntu 12.04 in VirtualBox and reproduced the error.
Looking in "config.log" shows what's going on.  It can be reproduced
with:

cat << EOF > foo.cc
#include <gtkmm.h>
int main()
{
  Gtk::Window mywindow;
  mywindow.set_default_icon_name("myappname");
  return 0;
}
EOF
g++ -o foo -g -O2 `pkg-config gtkmm-2.4 --cflags --libs` foo.cc

However this works:

g++ -o foo -g -O2 foo.cc `pkg-config gtkmm-2.4 --cflags --libs`

So it's the linker which is processing object code in strict order.
With foo.cc last there are no following libraries to resolve the
external symbols.  Could be (1) that the linker on other distributions
works out of order; (2) autoconf is passing the test program in the
wrong place on the command line or; (3) I've coded the autoconf check
for set_default_icon_name() slightly wrong and it happens to work on
other distributions.  Investigation continuing.
Comment 35 Mike Fleetwood 2013-04-29 12:27:33 UTC
Re-checked the results from CentOS 5.9.  It's using the same, supply the
test source file (conftest.cc) last so is relying on out of order
processing of libraries by the linker.  The test is not failing because
it can't link, but instead because the member function
set_default_icon_name() doesn't exist.

CentOS 5.9 config.log fragment:
configure:21912: checking for Gtk::Window::set_default_icon_name method
configure:21933: g++ -o conftest -g -O2 -I/usr/include/gtkmm-2.4 -I/usr/lib/gtkmm-2.4/include -I/usr/include/glibmm-2.4 -I/usr/lib/glibmm-2.4/include -I/usr/include/gdkmm-2.4 -I/usr/lib/gdkmm-2.4/include -I/usr/include/pangomm-1.4 -I/usr/include/atkmm-1.6 -I/usr/include/gtk-2.0 -I/usr/include/sigc++-2.0 -I/usr/lib/sigc++-2.0/include -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/lib/gtk-2.0/include -I/usr/include/cairomm-1.0 -I/usr/include/pango-1.0 -I/usr/include/cairo -I/usr/include/freetype2 -I/usr/include/libpng12 -I/usr/include/atk-1.0  -L/lib -lgtkmm-2.4 -lgdkmm-2.4 -latkmm-1.6 -lgtk-x11-2.0 -lpangomm-1.4 -lcairomm-1.0 -lglibmm-2.4 -lsigc-2.0 -lgdk-x11-2.0 -latk-1.0 -lgdk_pixbuf-2.0 -lm -lpangocairo-1.0 -lpango-1.0 -lcairo -lgobject-2.0 -lgmodule-2.0 -ldl -lglib-2.0     conftest.cc -ldl -luuid  >&5
conftest.cc: In function 'int main()':
conftest.cc:40: error: 'class Gtk::Window' has no member named 'set_default_icon_name'

So the linker on Kunbuntu 12.04 is behaving differently to anything else
that I have seen.
Comment 36 Phillip Susi 2013-04-29 13:07:56 UTC
IIRC, this was a change gcc made about the time of 12.04.
Comment 37 Curtis Gedak 2013-04-29 15:53:02 UTC
'Just one more piece of this puzzle.

The problem is not limited to KDE and occurs in Ubuntu 12.04 as well.  The reason I had not seen it before was because I had run "sudo make install".

As you correctly point out in comment #34, the "sudo make install" command will install the pixmap and hence the "Failed too open file..." message will not be displayed.

It is very interesting to note that the order of arguments seems to matter for these *buntu distros.

Distros that _do not_ correctly pass the configure check for
set_default_icon_name method are:

  Ubuntu 11.10, 12.04, 12.10, 13.04

Distros that _do_ correctly pass are:

  Ubuntu 10.04, 10.10, 11.04
  Debian 6
  Fedora 18
  OpenSUSE 12.2

Based on these distro tests, it would appear that there was some change between Ubuntu 11.04 and 11.10 that is incompatible with the configure check for the set_default_icon_name method.

With Ubuntu being a major distro, I think it is important to find a solution or workaround to this problem.
Comment 38 Mike Fleetwood 2013-04-29 16:22:03 UTC
I've done a quick manual compile of the check program and it looks like
changing the test for the set_default_icon_name method from a compile
and link (AC_LINK_IFELSE) to a compile only (AC_COMPILE_IFELSE) will
work.  Before I plump for this solution I would like to get to the
bottom of what changed in the linker with Ubuntu 11.10.  Thanks for
narrowing it down Curtis.
Comment 39 Phillip Susi 2013-04-29 17:58:08 UTC
Why not just change the order of the arguments?  I *think* this was an upstream change in gcc so will affect all distributions going forward.
Comment 40 Curtis Gedak 2013-04-29 19:11:24 UTC
The following link looks similar to the problem we have encountered.

c math linker problems on Ubuntu 11.10
http://stackoverflow.com/questions/7824439/c-math-linker-problems-on-ubuntu-11-10
Comment 41 Curtis Gedak 2013-04-29 19:42:04 UTC
It looks like Phillip is correct about the change in gcc default behaviour.

Ubuntu 11.10 Release Notes - GCC 4.6 Toolchain
https://wiki.ubuntu.com/OneiricOcelot/ReleaseNotes#GCC_4.6_Toolchain

Suggestions on how to address the problems introduced by this change can be found at the following link:

ToolchainTransition
https://wiki.ubuntu.com/NattyNarwhal/ToolchainTransition
Comment 42 Curtis Gedak 2013-04-29 19:58:49 UTC
Created attachment 242837 [details] [review]
Patch to fix change in default behaviour introduced with gcc 4.6

Mike,

This patch fixes the problem for me on kubuntu 12.04.  I will test this on other distros as well.

Can you also test this patch?

Thanks,
Curtis
Comment 43 Curtis Gedak 2013-04-29 20:23:59 UTC
The patch in comment #42 fixes the problem with the configure check for Gtk::Window::set_default_icon_name method in the following tested distros:

  Ubuntu 12.04, 13.04

The patch also works correctly with these following tested distros:

  Debian 6
  Fedora 18
  OpenSUSE 12.2

Note that I do not have a RHEL/CentOS 5.9 VM for testing.
Comment 44 Mike Fleetwood 2013-04-29 21:31:40 UTC
Created attachment 242849 [details] [review]
Fix set_default_icon_name autoconf check (v1)

Turns out that (1) and (3) are true.  (1) Ubuntu >= 11.10 uses linker
configuration that no one else uses.  (3) I have coded the autoconf
check slightly wrong.

Get the linker flags by adding '-###' to the g++/gcc command line.  It
prints the full command lines of all stages.  Summary:

OS			Linker flags
CentOS 5.9		[Neither]
Debian 6		[Neither]
Fedora 14		--no-add-needed
Fedora 17		--no-add-needed
Ubuntu 12.04		--no-add-needed --as-needed

The --no-add-needed (aka --no-copy-dt-needed-entries) says only add
shared libraries listed on the command line, not also any libraries they
depend on too.

The --as-needed says only add shared libraries which resolve undefined
references.  Since foo.cc was at the end the linker didn't discover the
needed external symbols until it has already passed all the libraries.
This is the unique difference in Ubuntu.


The fault with the autoconf check is that I was using the CXXFLAGS env
variable to pass the libraries, rather than LIBS so AC_LINK_IFELSE was
constructing the command line in the wrong order.


Fix attached.  Tested on CentOS 5.9, Debian 6, Fedora 14 & 17 and
Kbuntu 12.04.
Comment 45 Mike Fleetwood 2013-04-29 21:38:41 UTC
Oh dear clashing work.
Comment 46 Curtis Gedak 2013-04-29 21:40:09 UTC
No worries Mike.  More important is the finding the fix to this problem.  I'll do a quick review of your patch with the intention of committing it.  You patch has better comments.  :-)
Comment 47 Curtis Gedak 2013-04-29 21:49:58 UTC
Thank you Mike for your work to iron out these bugs.

The patch in comment #44 has been committed to the git repository for inclusion in the next release of GParted.

The relevant git commit can be viewed at the following link:

Fix autoconf check for set_default_icon_name method (#695279)
https://git.gnome.org/browse/gparted/commit/?id=b7b2af4f5eee7445c626b5f91f5890bd7fd3f3ca

Assuming that no more problems are discovered before tomorrow (and I do not expect any), then on Tuesday, April 30th I will package and release GParted 0.16.1.
Comment 48 Curtis Gedak 2013-04-30 16:30:52 UTC
The fix to address this bug report has been included in GParted 0.16.1 released on April 30, 2013.