From: Dima Kogan <dima@secretsauce.net>
To: Stefan Monnier <monnier@IRO.UMontreal.CA>
Cc: 18861@debbugs.gnu.org
Subject: bug#18861: Acknowledgement (25.0.50; gfile-based file notifications are not immediate)
Date: Wed, 05 Nov 2014 11:19:46 -0800 [thread overview]
Message-ID: <877fz9cv12.fsf@secretsauce.net> (raw)
In-Reply-To: <jwvtx2la6fl.fsf-monnier+emacsbugs@gnu.org>
[-- Attachment #1: Type: text/plain, Size: 1547 bytes --]
Stefan Monnier <monnier@IRO.UMontreal.CA> writes:
>> Hi. A glib maintainer responded to the bug report, and it turns out
>> emacs was using glib slightly incorrectly. I'm attaching a patch to fix
>> the issue, as suggested by the maintainer.
>
> Thanks. This is not my area of expertise at all, but since it seems
> that noone else has replied yet, I'll try to move it along.
>
>> + if( g_main_context_acquire(context) != TRUE )
>> + {
>> + // we couldn't acquire the context. I let this function proceed because it
>> + // handles more than just glib file descriptors
>> + retval = -1;
>> + }
>> + else
>> + context_acquired = true;
>
> [ Please follow our coding conventions: put a space before open parens
> (and not after, nor before close parens); use /*...*/ for comments;
> capitalize and punctuate comments; use two spaces between sentences.
> Also, I think "!g_main_context_acquire (context)" would be cleaner than
> "g_main_context_acquire (context) != TRUE". I find comparing to NULL,
> true, or false generally ugly, tho maybe that's just a personal taste
> of mine that others don't share. ]
>
> Why set retval to -1? It seems safer to leave it unchanged (ie. set to 0).
OK. How about the attached? The error-handling logic is better, and we
don't touch glib if we couldn't acquire the context. However in this
patch if we COULDN'T talk to glib, but we COULD talk to our own file
descriptors, then xg_select() returns success, and there is no error
reporting. Probably not what we want.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-xg_select-now-acquires-the-glib-context-before-query.patch --]
[-- Type: text/x-diff, Size: 2488 bytes --]
From 1fcced5536c8d99032303fb0f547aa67a5cccd30 Mon Sep 17 00:00:00 2001
From: Dima Kogan <dima@secretsauce.net>
Date: Tue, 28 Oct 2014 14:29:01 -0700
Subject: [PATCH] xg_select() now acquires the glib context before querying it
Prior to this patch we were calling g_main_context_query() without calling
g_main_context_acquire(). This resulted in the file descriptors returned by
g_main_context_query() missing activity. I.e. something would happen in glib,
but a select() on the file descriptors would keep blocking.
We now acquire the context, which makes select() return on activity, as it
should.
This is emacs and glib bugs:
http://debbugs.gnu.org/cgi/bugreport.cgi?bug=18861
https://bugzilla.gnome.org/show_bug.cgi?id=739274
---
src/xgselect.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/src/xgselect.c b/src/xgselect.c
index bf889a9..7dee38d 100644
--- a/src/xgselect.c
+++ b/src/xgselect.c
@@ -55,19 +55,28 @@ xg_select (int fds_lim, fd_set *rfds, fd_set *wfds, fd_set *efds,
GPollFD *gfds = gfds_buf;
int gfds_size = ARRAYELTS (gfds_buf);
int n_gfds, retval = 0, our_fds = 0, max_fds = fds_lim - 1;
- int i, nfds, tmo_in_millisec;
+ bool context_acquired = false;
+ int i, nfds, tmo_in_millisec = -1;
bool need_to_dispatch;
USE_SAFE_ALLOCA;
+ /* If we couldn't acquire the context, I let this function proceed because it
+ handles more than just glib file descriptors. Note that, as implemented,
+ this failure is completely silent: there is no feedback to the caller. */
context = g_main_context_default ();
+ if (g_main_context_acquire(context))
+ context_acquired = true;
if (rfds) all_rfds = *rfds;
else FD_ZERO (&all_rfds);
if (wfds) all_wfds = *wfds;
else FD_ZERO (&all_wfds);
- n_gfds = g_main_context_query (context, G_PRIORITY_LOW, &tmo_in_millisec,
- gfds, gfds_size);
+ n_gfds = context_acquired ?
+ g_main_context_query (context, G_PRIORITY_LOW, &tmo_in_millisec,
+ gfds, gfds_size) :
+ -1;
+
if (gfds_size < n_gfds)
{
SAFE_NALLOCA (gfds, sizeof *gfds, n_gfds);
@@ -152,6 +161,9 @@ xg_select (int fds_lim, fd_set *rfds, fd_set *wfds, fd_set *efds,
errno = pselect_errno;
}
+ if (context_acquired)
+ g_main_context_release(context);
+
/* To not have to recalculate timeout, return like this. */
if ((our_fds > 0 || (nfds == 0 && tmop == &tmo)) && (retval == 0))
{
--
2.0.0
next prev parent reply other threads:[~2014-11-05 19:19 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-10-28 0:29 bug#18861: 25.0.50; gfile-based file notifications are not immediate Dima Kogan
[not found] ` <handler.18861.B.14144561857404.ack@debbugs.gnu.org>
2014-10-28 4:22 ` bug#18861: Acknowledgement (25.0.50; gfile-based file notifications are not immediate) Dima Kogan
2014-10-28 21:30 ` Dima Kogan
2014-10-30 16:12 ` Stefan Monnier
2014-11-05 19:19 ` Dima Kogan [this message]
2014-11-06 3:02 ` Stefan Monnier
2014-11-06 8:37 ` Michael Albinus
2014-11-06 18:26 ` Dima Kogan
2014-11-06 23:35 ` Stefan Monnier
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=877fz9cv12.fsf@secretsauce.net \
--to=dima@secretsauce.net \
--cc=18861@debbugs.gnu.org \
--cc=monnier@IRO.UMontreal.CA \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.