Closed Bug 818106 Opened 7 years ago Closed 7 years ago

ScopedClose doesn't deal with EINTR

Categories

(Core :: XPCOM, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: dhylands, Assigned: badanomaly)

References

Details

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

Attachments

(1 file, 4 obsolete files)

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.
Whiteboard: [good first bug][mentor=dhylands][lang=c++]
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
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.
Attached patch Patch 1 (obsolete) — Splinter Review
Attachment #696998 - Flags: review?(dhylands)
I have attached the patch. Please Review it.
Attached patch Patch 2 (obsolete) — Splinter Review
Attachment #696998 - Attachment is obsolete: true
Attachment #696998 - Flags: review?(dhylands)
Attachment #697000 - Flags: review?(dhylands)
Attached patch Patch 3 (obsolete) — Splinter Review
Attachment #697000 - Attachment is obsolete: true
Attachment #697000 - Flags: review?(dhylands)
Attachment #697219 - Flags: review?(benjamin)
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-
Also:

nit: You should have have a space bewteen } and while and also between ) and {
Attached patch Patch 4 (obsolete) — Splinter Review
Attachment #697219 - Attachment is obsolete: true
Attachment #697276 - Flags: review?(dhylands)
Attachment #697276 - Flags: review?(dhylands) → review+
How do I get this bug assigned to me. I have attached the patch still its showing assigned to: nobody ?
In bugzilla, click on the "take" link next to the "Assigned To" and then click on Save.
(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 ?
Are you logged in?
There should be 2 links, one says edit, and one says take.
Assignee: nobody → mohit.www
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
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)
Are you on IRC? It's probably easier to have a discussion there, rather than in the bug...
Assignee: nobody → mohit.www
Status: NEW → ASSIGNED
Attachment #697763 - Flags: review?(dhylands)
Attachment #697276 - Attachment is obsolete: true
Attachment #697763 - Flags: review?(dhylands) → review+
https://hg.mozilla.org/mozilla-central/rev/b0967de80fb7
Status: ASSIGNED → RESOLVED
Closed: 7 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.
(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.