Fix extsmaild and inotify.

[ltratt]

Change from Stephen Kell in #51.

@oliv3 Are you able to double check this still keeps things working for you? I can't really test this on Linux.

[oliv3]

Hi Laurie,

I'll give it a try when I can get some time, $WORK is keeping me busy.

On January 5, 2022 7:20:55 PM GMT+01:00, Laurence Tratt @.***> wrote:

Change from Stephen Kell in #51.

@.*** Are you able to double check this still keeps things working for you? I can't really test this on Linux.

You can view, comment on, or merge this pull request online at:

https://github.com/ltratt/extsmail/pull/52

-- Commit Summary --

• Fix extsmaild and inotify.

-- File Changes --

M extsmaild.c (3)

-- Patch Links --

https://github.com/ltratt/extsmail/pull/52.patch https://github.com/ltratt/extsmail/pull/52.diff

-- Reply to this email directly or view it on GitHub: https://github.com/ltratt/extsmail/pull/52 You are receiving this because you were assigned.

Message ID: @.***>

-- Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.

[oliv3]

From a quick glance, I would say it looks ok, still the return of pselect() should be checked for errors (-1)

On January 5, 2022 7:20:55 PM GMT+01:00, Laurence Tratt @.***> wrote:

Change from Stephen Kell in #51.

@.*** Are you able to double check this still keeps things working for you? I can't really test this on Linux.

You can view, comment on, or merge this pull request online at:

https://github.com/ltratt/extsmail/pull/52

-- Commit Summary --

• Fix extsmaild and inotify.

-- File Changes --

M extsmaild.c (3)

-- Patch Links --

https://github.com/ltratt/extsmail/pull/52.patch https://github.com/ltratt/extsmail/pull/52.diff

-- Reply to this email directly or view it on GitHub: https://github.com/ltratt/extsmail/pull/52 You are receiving this because you were assigned.

Message ID: @.***>

-- Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.

[ltratt]

In this case, I think it's OK not to check with errors on the basis that it's better to wake up more often than less often -- and since I don't think we can do anything useful in the face of an error anyway. I'm happy to be corrected however!

[oliv3]

All system calls MUST have their return value checked for errors, or mayhem can happen. So even if we decide to ignore them, a failure should be at least syslog()-ed, to help debugging if one day things go wrong. I'd suggest something like:

int ret = pselect(...);
if (ret == -1) {
  syslog(LOG_ERR, "%s: pselect failed: %s", __func__, strerror(errno));
} else if (ret == 1) {
  // do things
}

[ltratt]

Normally I'd agree, but I'm neutral on pselect failing (similarly kqueue's return code is ignored on platforms that have kqueue), because its failure isn't super important. But I'm happy to go with whatever your (@oliv3) and @stephenrkell think appropriate.

[oliv3]

ENOMEM usually means bad things.

[ltratt]

I had to check that because posix doesn't say that pselect can return ENOMEM. It seems that's a Linux oddity that everyone else handles as EAGAIN. That does suggest to me that even with pselect, ENOMEM isn't that severe. But, again, since I'm not using inotify or pselect, I'm happy to be led by those who do :) Someone's going to have to write the code though, or merge this PR and raise a subsequent one...

[stephenrkell]

I agree there's a code quality issue around error checking. However, it seems possible to separate that out from this issue, which is a matter of functionality. The old code wasn't doing anything with an ENOMEM (or other error) returned from pselect, just carrying on after skipping the read.

(I was a bit surprised that Linux's pselect() can return ENOMEM, since conceptually it doesn't require the kernel to allocate anything... but I am guessing there is some lazy allocation going on.)

[oliv3]

Adding two lines (pselect() returns -1 => syslog() that), or do the right thing in case there was no error shouldn't be that hard to add to this PR IMHO.

[ltratt]

Don't forget: I can't run this code (I just copy and pasted Stephen's code from #51). If someone wants to add additional code, they need to tell me exactly what it should be.

[ltratt]

@oliv3 @stephenrkell I've pushed a change which logs pselect() failures. I can't even compile this so please can some check if it at least compiles? Thanks!

[ltratt]

Can one of you perhaps try compiling and suggest a fix? There's a very hard limit to my ability to debug this one!

[stephenrkell]

I have compiled with the following small patch. Am now running this version!

diff --git a/extsmaild.c b/extsmaild.c
index 50b2b13..4bd801c 100644
--- a/extsmaild.c
+++ b/extsmaild.c
@@ -1621,8 +1621,8 @@ int main(int argc, char** argv)
                 int res = read(fd, buf, INOTIFY_BUFLEN);
                 if (res == -1)
                     syslog(LOG_ERR, "Error when reading from inotify buffer");
-            } else (rtn == -1) {
-                syslog(LOG_ERR, "main: pselect: %s", strerr(errno));
+            } else if (rtn == -1) {
+                syslog(LOG_ERR, "main: pselect: %s", strerror(errno));
             }
 #else
             // If no other support is available, we fall back on polling alone.

[ltratt]

OK, pushed Stephen's suggested fix.

@oliv3 Are you OK with this? If so, I would like to squash before we consider merging :)

[oliv3]

Looks good to me, but I won't be able to test this soon, so if/when @stephenrkell confirms it's ok, I'd say go for it.

[ltratt]

@stephenrkell I've squashed this down into a commit with you as the main author and me as the co-author. If you're OK with how this looks, then I think we'll be good for @oliv3 to merge!

[stephenrkell]

Yep, happy -- thanks!

[ltratt]

@oliv3 If you're happy, please merge :)