Last Comment Bug 818106 - ScopedClose doesn't deal with EINTR
: ScopedClose doesn't deal with EINTR
Status: RESOLVED FIXED
[good first bug][mentor=dhylands][lan...
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla20
Assigned To: [:badanomaly]
:
Mentors:
: 864524 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-12-04 09:16 PST by Dave Hylands [:dhylands]
Modified: 2013-04-23 09:27 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch 1 (5.78 KB, patch)
2013-01-02 05:45 PST, [:badanomaly]
no flags Details | Diff | Review
Patch 2 (745 bytes, patch)
2013-01-02 05:52 PST, [:badanomaly]
no flags Details | Diff | Review
Patch 3 (1.07 KB, patch)
2013-01-02 14:33 PST, [:badanomaly]
dhylands: review-
Details | Diff | Review
Patch 4 (1.06 KB, patch)
2013-01-02 17:00 PST, [:badanomaly]
dhylands: review+
Details | Diff | Review
Fix: ScopedClose now considers errno EINTR. (1.27 KB, patch)
2013-01-03 18:48 PST, [:badanomaly]
dhylands: review+
Details | Diff | Review

Description Dave Hylands [:dhylands] 2012-12-04 09:16:30 PST
close can fail with errno == EINTR, in which case the close call needs to be made again.

ScopedClose just calls close and doen't check the return code, assuming that the close succeeded. If the close returns EINTR, then the close may not have actually happened.
Comment 1 Josh Matthews [:jdm] 2012-12-18 20:06:01 PST
For whomever takes this one, see http://hg.mozilla.org/mozilla-central/annotate/287a7d7cf7f0/xpcom/glue/FileUtils.h#l54
Comment 2 gautam4you 2012-12-21 11:22:06 PST
Just changing the close(fd); to while(close(fd)==EINTR); is enough ? 
Or should there be any maximum retries kind of logic ? 
Or should it wait for some time before retrying again ? 
Please let me know the details
Comment 3 Dave Hylands [:dhylands] 2012-12-21 11:53:17 PST
The actual logic should be:

while ((close(fd) == -1) && (errno == EINTR)) {
    ;
}

You shouldn't need any maximum retries logic. This is essentially the same logic found in the
TEMP_FAILURE_RETRY macro from /usr/include/unistd.h, but that macro is glibc specific, so it may not be portable across all of the platforms that we support.
Comment 4 [:badanomaly] 2013-01-02 05:45:26 PST
Created attachment 696998 [details] [diff] [review]
Patch 1
Comment 5 [:badanomaly] 2013-01-02 05:48:18 PST
I have attached the patch. Please Review it.
Comment 6 [:badanomaly] 2013-01-02 05:52:41 PST
Created attachment 697000 [details] [diff] [review]
Patch 2
Comment 7 [:badanomaly] 2013-01-02 14:33:34 PST
Created attachment 697219 [details] [diff] [review]
Patch 3
Comment 8 Dave Hylands [:dhylands] 2013-01-02 15:16:37 PST
Comment on attachment 697219 [details] [diff] [review]
Patch 3

Review of attachment 697219 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/glue/FileUtils.h
@@ +49,5 @@
>    static void release(type fd) {
> +    if (fd != -1){
> +      do{
> +        close(fd);
> +      }while (errno == EINTR);

This won't work. errno isn't set if close returns successfully. You should only test errno if close returns -1, or if you explicitly set errno to 0 prior to calling close.
Comment 9 Dave Hylands [:dhylands] 2013-01-02 15:19:01 PST
Also:

nit: You should have have a space bewteen } and while and also between ) and {
Comment 10 [:badanomaly] 2013-01-02 17:00:22 PST
Created attachment 697276 [details] [diff] [review]
Patch 4
Comment 11 [:badanomaly] 2013-01-02 17:17:34 PST
How do I get this bug assigned to me. I have attached the patch still its showing assigned to: nobody ?
Comment 12 Dave Hylands [:dhylands] 2013-01-02 17:19:11 PST
In bugzilla, click on the "take" link next to the "Assigned To" and then click on Save.
Comment 13 [:badanomaly] 2013-01-02 17:24:48 PST
(In reply to Dave Hylands [:dhylands] from comment #12)
> In bugzilla, click on the "take" link next to the "Assigned To" and then
> click on Save.

clicking on the link creates a new email addressed to nobody@mozilla.org .
Am i supposed to send an email ?
Comment 14 Dave Hylands [:dhylands] 2013-01-02 17:37:57 PST
Are you logged in?
There should be 2 links, one says edit, and one says take.
Comment 15 Dave Hylands [:dhylands] 2013-01-02 17:39:45 PST
Oops - I wound up assigning it to you. I was playing with logging in and out, and wound up assigning it to you.

So I'll assign it back to nobody.
Comment 16 Dave Hylands [:dhylands] 2013-01-02 17:43:19 PST
If you click on edit, it will shows who its currently assigned to, and you can change it (if you have permission). You should be able to change it to your email address.

Clicking on take should fill in your email address (so this should be easier than clicking on edit)
Comment 17 Dave Hylands [:dhylands] 2013-01-02 17:44:41 PST
Are you on IRC? It's probably easier to have a discussion there, rather than in the bug...
Comment 18 Dave Hylands [:dhylands] 2013-01-02 19:31:23 PST
Try:
https://tbpl.mozilla.org/?tree=Try&rev=cfc2657ae40c
Comment 19 [:badanomaly] 2013-01-03 18:48:49 PST
Created attachment 697763 [details] [diff] [review]
Fix: ScopedClose now considers errno EINTR.
Comment 21 Ed Morley [:emorley] 2013-01-04 09:50:23 PST
https://hg.mozilla.org/mozilla-central/rev/b0967de80fb7
Comment 22 Justin Lebar (not reading bugmail) 2013-04-23 07:37:19 PDT
*** Bug 864524 has been marked as a duplicate of this bug. ***
Comment 23 Justin Lebar (not reading bugmail) 2013-04-23 07:38:50 PDT
> If you click on edit

Note that people new to bugzilla don't initially have permission to edit many of a bug's metadata fields.  You can go on IRC and ask someone to give X e-mail address "editbugs" permission.
Comment 24 Dave Hylands [:dhylands] 2013-04-23 09:27:28 PDT
(In reply to Justin Lebar [:jlebar] from comment #23)
> > If you click on edit
> 
> Note that people new to bugzilla don't initially have permission to edit
> many of a bug's metadata fields.  You can go on IRC and ask someone to give
> X e-mail address "editbugs" permission.

Yeah - we eventually figured that out, and now that I'm aware that's one of the things I look for. Apparently, the proper procedure for getting editbugs is to follow this:
https://bugzilla.mozilla.org/page.cgi?id=get_permissions.html

and this page is generally a good page to read when first learning about bugzilla:
https://developer.mozilla.org/en/docs/What_to_do_and_what_not_to_do_in_Bugzilla

Note You need to log in before you can comment on or make changes to this bug.