The default bug view has changed. See this FAQ.

ScopedClose doesn't deal with EINTR

RESOLVED FIXED in mozilla20

Status

()

Core
XPCOM
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: dhylands, Assigned: badanomaly)

Tracking

unspecified
mozilla20
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=dhylands][lang=c++])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Updated

4 years ago
Whiteboard: [good first bug][mentor=dhylands][lang=c++]

Comment 1

4 years ago
For whomever takes this one, see http://hg.mozilla.org/mozilla-central/annotate/287a7d7cf7f0/xpcom/glue/FileUtils.h#l54

Comment 2

4 years ago
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
(Reporter)

Comment 3

4 years ago
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.
(Assignee)

Comment 4

4 years ago
Created attachment 696998 [details] [diff] [review]
Patch 1
Attachment #696998 - Flags: review?(dhylands)
(Assignee)

Comment 5

4 years ago
I have attached the patch. Please Review it.
(Assignee)

Comment 6

4 years ago
Created attachment 697000 [details] [diff] [review]
Patch 2
Attachment #696998 - Attachment is obsolete: true
Attachment #696998 - Flags: review?(dhylands)
Attachment #697000 - Flags: review?(dhylands)
(Assignee)

Comment 7

4 years ago
Created attachment 697219 [details] [diff] [review]
Patch 3
Attachment #697000 - Attachment is obsolete: true
Attachment #697000 - Flags: review?(dhylands)
Attachment #697219 - Flags: review?(benjamin)
(Reporter)

Comment 8

4 years ago
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.
Attachment #697219 - Flags: review?(benjamin) → review-
(Reporter)

Comment 9

4 years ago
Also:

nit: You should have have a space bewteen } and while and also between ) and {
(Assignee)

Comment 10

4 years ago
Created attachment 697276 [details] [diff] [review]
Patch 4
Attachment #697219 - Attachment is obsolete: true
Attachment #697276 - Flags: review?(dhylands)
(Reporter)

Updated

4 years ago
Attachment #697276 - Flags: review?(dhylands) → review+
(Assignee)

Comment 11

4 years ago
How do I get this bug assigned to me. I have attached the patch still its showing assigned to: nobody ?
(Reporter)

Comment 12

4 years ago
In bugzilla, click on the "take" link next to the "Assigned To" and then click on Save.
(Assignee)

Comment 13

4 years ago
(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 ?
(Reporter)

Comment 14

4 years ago
Are you logged in?
There should be 2 links, one says edit, and one says take.
Assignee: nobody → mohit.www
(Reporter)

Comment 15

4 years ago
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.
Assignee: mohit.www → nobody
(Reporter)

Comment 16

4 years ago
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)
(Reporter)

Comment 17

4 years ago
Are you on IRC? It's probably easier to have a discussion there, rather than in the bug...
(Reporter)

Updated

4 years ago
Assignee: nobody → mohit.www
(Reporter)

Comment 18

4 years ago
Try:
https://tbpl.mozilla.org/?tree=Try&rev=cfc2657ae40c
(Assignee)

Comment 19

4 years ago
Created attachment 697763 [details] [diff] [review]
Fix: ScopedClose now considers errno EINTR.
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

4 years ago
Attachment #697763 - Flags: review?(dhylands)
(Assignee)

Updated

4 years ago
Attachment #697276 - Attachment is obsolete: true
(Reporter)

Updated

4 years ago
Attachment #697763 - Flags: review?(dhylands) → review+
(Reporter)

Comment 20

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0967de80fb7
https://hg.mozilla.org/mozilla-central/rev/b0967de80fb7
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Duplicate of this bug: 864524
> 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.
(Reporter)

Comment 24

4 years ago
(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
You need to log in before you can comment on or make changes to this bug.