Change in vdsm[master]: remoteFileHandler: Add create exclusive option for truncateFile

Yeela Kaplan has uploaded a new change for review.

Change subject: remoteFileHandler: Add create exclusive option for truncateFile
......................................................................

remoteFileHandler: Add create exclusive option for truncateFile

Change-Id: Idfeff348e0f6fc240954e7d304b794dd99ea098c
Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=979193
Signed-off-by: Yeela Kaplan <ykaplan(a)redhat.com&gt;
---
M vdsm/storage/remoteFileHandler.py
1 file changed, 7 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/22/19022/1

diff --git a/vdsm/storage/remoteFileHandler.py b/vdsm/storage/remoteFileHandler.py
index 779b045..f01461f 100644
--- a/vdsm/storage/remoteFileHandler.py
+++ b/vdsm/storage/remoteFileHandler.py
@@ -347,8 +347,13 @@
         return f.writelines(lines)
 
 
-def truncateFile(path, size, mode=None):
-    with open(path, "w") as f:
+def truncateFile(path, size, mode=None, creatExcl=False):
+    flags = os.O_CREAT | os.O_WRONLY
+    if creatExcl:
+        flags |= os.O_EXCL
+
+    fd = os.open(path, flags)
+    with os.fdopen(fd, 'w') as f:
         if mode is not None:
             os.chmod(path, mode)
         f.truncate(size)


-- 
To view, visit http://gerrit.ovirt.org/19022
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Idfeff348e0f6fc240954e7d304b794dd99ea098c
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yeela Kaplan <ykaplan(a)redhat.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: remoteFileHandler: Add create exclusive option for truncateFile
......................................................................


remoteFileHandler: Add create exclusive option for truncateFile

Change-Id: Idfeff348e0f6fc240954e7d304b794dd99ea098c
Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=979193
Signed-off-by: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Reviewed-on: http://gerrit.ovirt.org/19022
Reviewed-by: Dan Kenigsberg <danken(a)redhat.com&gt;
---
M vdsm/storage/remoteFileHandler.py
1 file changed, 7 insertions(+), 2 deletions(-)

Approvals:
  Yeela Kaplan: Verified
  Dan Kenigsberg: Looks good to me, approved



-- 
To view, visit http://gerrit.ovirt.org/19022
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Idfeff348e0f6fc240954e7d304b794dd99ea098c
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: remoteFileHandler: Add create exclusive option for truncateFile
......................................................................


Patch Set 1: Code-Review+1

(1 comment)

....................................................
File vdsm/storage/remoteFileHandler.py
Line 351:     flags = os.O_CREAT | os.O_WRONLY
Line 352:     if creatExcl:
Line 353:         flags |= os.O_EXCL
Line 354: 
Line 355:     fd = os.open(path, flags)
Oh, sorry Yeela, no leak.
I personally prefer Python-level managing of IO calls over direct POSIX.
Sorry for the noise.
Line 356:     with os.fdopen(fd, 'w') as f:
Line 357:         if mode is not None:
Line 358:             os.chmod(path, mode)
Line 359:         f.truncate(size)


-- 
To view, visit http://gerrit.ovirt.org/19022
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idfeff348e0f6fc240954e7d304b794dd99ea098c
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: remoteFileHandler: Add create exclusive option for truncateFile
......................................................................


Patch Set 1:

(1 comment)

....................................................
File vdsm/storage/remoteFileHandler.py
Line 351:     flags = os.O_CREAT | os.O_WRONLY
Line 352:     if creatExcl:
Line 353:         flags |= os.O_EXCL
Line 354: 
Line 355:     fd = os.open(path, flags)
no, the file object returned by os.fdopen(...) is going to call close at the end of the
"with" block.
We can leak only if os.fdopen(...) raises an exception (instead of returning the file
object), but I don't see how.

Anyway we can try to avoid os.fdopen(...) using all posix-like calls:

 fd = os.open(path, flags)
 try:
     if mode is not None:
         os.fchmod(fd, mode)
     os.ftruncate(fd, size)
 finally:
     os.close(fd)

In this specific case (as os.fdopen might be confusing) this implementation looks better
to me.
Line 356:     with os.fdopen(fd, 'w') as f:
Line 357:         if mode is not None:
Line 358:             os.chmod(path, mode)
Line 359:         f.truncate(size)


-- 
To view, visit http://gerrit.ovirt.org/19022
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idfeff348e0f6fc240954e7d304b794dd99ea098c
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: remoteFileHandler: Add create exclusive option for truncateFile
......................................................................


Patch Set 1:

Build Successful 

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/4331/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/3434/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/4250/ : SUCCESS

-- 
To view, visit http://gerrit.ovirt.org/19022
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idfeff348e0f6fc240954e7d304b794dd99ea098c
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: remoteFileHandler: Add create exclusive option for truncateFile
......................................................................


Patch Set 1: Code-Review-1

(1 comment)

....................................................
File vdsm/storage/remoteFileHandler.py
Line 351:     flags = os.O_CREAT | os.O_WRONLY
Line 352:     if creatExcl:
Line 353:         flags |= os.O_EXCL
Line 354: 
Line 355:     fd = os.open(path, flags)
file descriptor leak! you should call os.close(fd) on a "finally" clause.
Line 356:     with os.fdopen(fd, 'w') as f:
Line 357:         if mode is not None:
Line 358:             os.chmod(path, mode)
Line 359:         f.truncate(size)


-- 
To view, visit http://gerrit.ovirt.org/19022
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idfeff348e0f6fc240954e7d304b794dd99ea098c
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: remoteFileHandler: Add create exclusive option for truncateFile
......................................................................


Patch Set 1: Code-Review+2

-- 
To view, visit http://gerrit.ovirt.org/19022
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idfeff348e0f6fc240954e7d304b794dd99ea098c
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Yeela Kaplan has posted comments on this change.

Change subject: remoteFileHandler: Add create exclusive option for truncateFile
......................................................................


Patch Set 1: Verified+1

-- 
To view, visit http://gerrit.ovirt.org/19022
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idfeff348e0f6fc240954e7d304b794dd99ea098c
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No