GNOME Bugzilla – Bug 772074
Invalid type of file descriptor related methods in PollFD
Last modified: 2016-12-12 08:34:03 UTC
'fd' field of the GPollFD can have either gint, or gint64 type. glibmm always use int, therefore user can get conversion warnings on compilation. I think we should fix it in glibmm.
Created attachment 336379 [details] [review] proposed solution
Yes, this ought to be fixed in glibmm. Some comments, though: Your patch changes main.h, but it does not change main.cc. Some "int"s shall be replaced by "fd_t" in main.cc, too. We usually use the order "public, protected, private" in class definitions. I like this order. Your patch changes it in the PollFD class. Is this only a question of compiler warnings, or is it more serious? Can a conversion from gint64 to gint remove important information? gint64 is used only in Windows, where fd is a handle (according to a comment in glib/glib/gpoll.h). If a handle is actually an address, conversion from gint64 to gint can probably cause a program crash later. A fix will break ABI on 64-bit Windows systems. Is that acceptable now?
Created attachment 337526 [details] [review] proposed solution v2 (In reply to Kjell Ahlstedt from comment #2) > Your patch changes main.h, but it does not change main.cc. Some "int"s shall > be > replaced by "fd_t" in main.cc, too. Right, fixed > We usually use the order "public, protected, private" in class definitions. > I like this order. Your patch changes it in the PollFD class. I didn't know that it's possible to access the field without the instance in decltype, but apparently, it is, so I've fixed it. > Is this only a question of compiler warnings, or is it more serious? Can a > conversion from gint64 to gint remove important information? > gint64 is used only in Windows, where fd is a handle (according to a comment > in > glib/glib/gpoll.h). If a handle is actually an address, conversion from > gint64 > to gint can probably cause a program crash later. Yes, it can be more serious than just a warning. > A fix will break ABI on 64-bit Windows systems. Is that acceptable now? I don't know, I think we should ask Murray about it.
Your patch looks good. I thinks it's reasonable to push it despite the ABI break, but as you say, we should let Murray decide.
gtkmm 3 on Windows is basically an unstable platform at the moment anyway. But is this likely to affect anyone on Windows using gtkmm-2.4? Should we ask on the mailing list?
Yes, it can probably affect some users of gtkmm-2.4. We've seen lately that some users of gtkmm-2.4 want to combine it with a new version of glibmm (bug 770682). And I suppose there might be people who use glibmm without using gtkmm.
So what we're going to do with the patch then?
Is anyone likely to be using this API, even on Linux?
And is it even possible to be using it on 64-bit Windows at the moment?
I think glibmm/gtkmm is not very popular on Windows platform, so it's very unlikely that someone's using this particular API. However, that's just my guess.
File descriptors are used in Glib::Dispatcher. glibmm/glib/glibmm/dispatcher.cc should also be changed. I didn't notice that before. In DispatchNotifier's constructor, a handle is converted to an int. Probably it's ok to do that in a 32-bit system, but it might be disastrous in a 64-bit system. Perhaps more changes are necessary in dispatcher.cc. Apart from that, I also think that this API is unlikely to be used.
Do you mind if I push the patch then?
Yes, please. Could you try to fix Dispatcher too?
I have pushed Marcin's patch to the master branch. (Actually an updated version of it. It didn't apply because of other recent changes in main.cc.) I have also pushed a patch that fixes Dispatcher. Murray, shall these patches be pushed to the glibmm-2-50 branch?
The question remains: Shall these patches be pushed to the glibmm-2-50 branch? Bug 774903 comment 4 says that we shall not add API to the glibmm-2-50 branch. This is different. It does not add API. It breaks ABI, but only for 64-bit Windows programs. And it only breaks a part of the ABI which is wrong and probably useless for those programs.
Yes, please go ahead. I'm sorry for not responding quickly enough - please feel free to do what you think is best if I don't respond. I should never get in the way. I trust your judgement and I can always revert something if I object strongly. Thanks.
Fixed. Pushed the two patches to the glibmm-2-50 branch.