GNOME Bugzilla – Bug 746628
ghandle: introduce new ghandle type
Last modified: 2018-05-24 17:42:49 UTC
Add a new type that corresponds to a HANDLE on Windows and a file descriptor (int) on UNIX. Use it in the definition of #GPollFD.
Created attachment 300108 [details] [review] ghandle: introduce new ghandle type
Comment on attachment 300108 [details] [review] ghandle: introduce new ghandle type It's difficult to say for sure if this is right or wrong in isolation. We probably want to review the whole branch before committing any of it... >+ * Copyright © 2015 Canonical Limited ("©" == "Copyright", so "Copyright ©" == "Copyright Copyright". You don't need/want the symbol if you have the word.) >+ * #ghandle is an abstraction for some file-like objects. "for some pollable objects" is probably more accurate >+ * Since: 2.44 you missed some s/44/46/ >+ * g_handle_dup: Is DuplicateHandle() sufficiently semantically identical to dup() for this to work? It looks like it has the same behaviors wrt file offset and closing, and I can't think of anything else off the top of my head that would matter, so maybe we're ok? Well, except: >+ * This call may fail in response to system limits or resource >+ * allocation failures. The Windows docs also say that certain handle types are un-dup-able. (Though it's not clear if there are cases where it actually returns an error rather than returning an unusable handle...)
(In reply to Dan Winship from comment #2) > Comment on attachment 300108 [details] [review] [review] > ghandle: introduce new ghandle type > > It's difficult to say for sure if this is right or wrong in isolation. We > probably want to review the whole branch before committing any of it... > > >+ * Copyright © 2015 Canonical Limited > > ("©" == "Copyright", so "Copyright ©" == "Copyright Copyright". You don't > need/want the symbol if you have the word.) OK > "for some pollable objects" is probably more accurate OK > >+ * Since: 2.44 > you missed some s/44/46/ Thanks > >+ * g_handle_dup: > > Is DuplicateHandle() sufficiently semantically identical to dup() for this > to work? It looks like it has the same behaviors wrt file offset and > closing, and I can't think of anything else off the top of my head that > would matter, so maybe we're ok? Well, except: > > >+ * This call may fail in response to system limits or resource > >+ * allocation failures. > > The Windows docs also say that certain handle types are un-dup-able. (Though > it's not clear if there are cases where it actually returns an error rather > than returning an unusable handle...) Sockets are the biggest example of that that I know of. If we plan mostly to use this with pollable things then I think we're going to be OK.
Review of attachment 300108 [details] [review]: ::: glib/ghandle.c @@ +164,3 @@ + * + * It is completely unspecified whether the handle returned by this call + * will be closed on exec(). Why document it as undefined, when you can just return a CLOEXEC'd FD ? @@ +176,3 @@ + +#ifdef G_OS_UNIX + result = dup (handle); As per above, this should be sth like result = fcntl(handle, F_DUPFD_CLOEXEC, 0) ::: glib/ghandle.h @@ +5,3 @@ + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2 of the licence, or (at your option) any later version. Lesser only exists in version 2.1, not 2 @@ +39,3 @@ +typedef gint ghandle; +#define G_HANDLE_INVALID -1 +#define G_HANDLE_FORMAT "%d" Would be nice to be consistent with all other int formats where the G_*_FORMAT define does *not* include the '%'.
(In reply to Christian Persch from comment #4) > Why document it as undefined, when you can just return a CLOEXEC'd FD ? Because I didn't know how to do that at the time :) I'm also starting to become convinced that O_CLOEXEC may not be a great default.. > Lesser only exists in version 2.1, not 2 This is a mistake that exists across a lot of files, then... > @@ +39,3 @@ > +typedef gint ghandle; > +#define G_HANDLE_INVALID -1 > +#define G_HANDLE_FORMAT "%d" > > Would be nice to be consistent with all other int formats where the > G_*_FORMAT define does *not* include the '%'. Huh. Odd. I copied from (the poorly-named) G_POLLFD_FORMAT which does include the "%". I agree, though.
Created attachment 300150 [details] [review] ghandle: introduce new ghandle type Adjusted as per comments, and actually tried building it on Windows (which showed that some headers were missing).
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/1017.