1135255 - devicemanagerADB doesn't close mkstemp() file handles

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Michael Vines [:m1] [:evilmachines]]

tempfile.mkstemp() returns a file descriptor that the caller is expected to close.  Not doing this can cause trouble on platforms such as Windows where it's not permitted to delete an open file

Comment 1

[Michael Vines [:m1] [:evilmachines]]

Attachment: 0001-Bug-1135255-Close-the-fd-that-mkstemp-returns.patch

Comment 2

[William Lachance (:wlach)]

Comment on attachment 8567329 [details] [diff] [review]
0001-Bug-1135255-Close-the-fd-that-mkstemp-returns.patch

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

This is reasonable but I think I'd prefer to use "with". i.e.

with tempfile.NamedTemporarilyFile() as f:
   do stuff with f.name
   ...

This should auto-close the file and be a lot cleaner than what we have right now to boot.

Comment 3

[Michael Vines [:m1] [:evilmachines]]

Ah, yep that would be better.  Thanks for the quick feedback.  Turns out it's easier for us to maintain this as a local patch ATM than upstream so unassigning myself.

Comment 4

[William Lachance (:wlach)]

Attachment: Fix mozdevice tempfile handling on Windows

I figured we may as well fix this properly for next time, while it's fresh in our minds. This uses mozfile's NamedTemporaryFile that should give us the behaviour we want on Windows.

Comment 5

[William Lachance (:wlach)]

try run for b2g to make sure stuff still works: https://treeherder.mozilla.org/#/jobs?repo=try&revision=abc1d2286d56

Comment 6

[William Lachance (:wlach)]

Try run looks good, let's land this.

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6e9ecd2bed0

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c6e9ecd2bed0

Comment 9

[Michael Vines [:m1] [:evilmachines]]

Thanks guys