Differential D786

createhdds: use temp file correctly when uploading
ClosedPublic
Actions

Authored by adamwill on Mar 19 2016, 12:11 AM.

Details

Reviewers
garretraziel
jskladan
Commits
rOPENQA77d8eead27cc: createhdds: use temp file correctly when uploading
Summary

With the previous code, the temporary file wasn't closed before
we try to upload its contents to the disk image. This seems to
cause problems when run in Python 2; the file is truncated on
upload. So rejig things a bit so we use tempfile.mkstemp, close
the file after writing to it, then remove it ourselves after
doing the upload. Tested with Python 2 and Python 3 that this
creates a correct image.

If something goes wrong at the wrong time the temp file will
be left around, but hey, it's a temp file - they get cleaned
up on system shutdown. So doesn't seem like a big deal.

Test Plan

Check building the two images that involve a file
upload in both Python 2 and Python 3, make sure the file is
complete and correct in all cases.

Diff Detail

Repository
rOPENQA fedora_openqa
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
adamwill retitled this revision from to createhdds: use temp file correctly when uploading.Mar 19 2016, 12:11 AM
adamwill updated this object.
adamwill edited the test plan for this revision. (Show Details)
adamwill added reviewers: jskladan, garretraziel.
adamwill updated this revision to Diff 2007.Mar 19 2016, 9:26 PM

call gfs.shutdown() as well as gfs.close() (per rwmj)

see https://bugzilla.redhat.com/show_bug.cgi?id=1319347#c6

garretraziel accepted this revision.Mar 21 2016, 1:49 PM

Works OK.

This revision is now accepted and ready to land.Mar 21 2016, 1:49 PM
Closed by commit rOPENQA77d8eead27cc: createhdds: use temp file correctly when uploading (authored by adamwill). · Explain WhyMar 21 2016, 3:53 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathPackages
Mtools/createhdds.py (19 lines)

Diff 2010

View Options

tools/createhdds.py

Show First 20 Lines • Show All 141 Lines • ▼ Show 20 Line(s)134 for write in self.writes:
142 gfs.write(write['path'], write['content']) 142​ gfs.write(write['path'], write['content'])
143 # do file 'uploads'. in guestfs-speak that means transfer 143​ # do file 'uploads'. in guestfs-speak that means transfer
144 # a file from the host to the image, we use it to mean 144​ # a file from the host to the image, we use it to mean
145 # download a file from an http server and transfer that 145​ # download a file from an http server and transfer that
146 # to the image 146​ # to the image
147 for upload in self.uploads: 147​ for upload in self.uploads:
148 # download the file to a temp file - borrowed from 148​ # download the file to a temp file - borrowed from
149 # fedora_openqa_schedule (which stole it from SO) 149​ # fedora_openqa_schedule (which stole it from SO)
150 with tempfile.NamedTemporaryFile(prefix="createhdds") as dltmpfile: 150​ (_, tmpfname) = tempfile.mkstemp(prefix="createhdds")
151​ with open(tmpfname, 'wb') as tmpfh:
151 resp = urlopen(upload['source']) 152​ resp = urlopen(upload['source'])
152 while True: 153​ while True:
153 # This is the number of bytes to read between buffer 154​ # This is the number of bytes to read between buffer
154 # flushes. Value taken from the SO example. 155​ # flushes. Value taken from the SO example.
155 buff = resp.read(8192) 156​ buff = resp.read(8192)
156 if not buff: 157​ if not buff:
157 break 158​ break
158 dltmpfile.write(buff) 159​ tmpfh.write(buff)
159 # as with write, the dict must specify a target 160​ # as with write, the dict must specify a target
160 # partition and location ('target') 161​ # partition and location ('target')
161 partn = gfs.list_partitions()[int(upload['part'])-1] 162​ partn = gfs.list_partitions()[int(upload['part'])-1]
162 gfs.mount(partn, "/") 163​ gfs.mount(partn, "/")
163 gfs.upload(dltmpfile.name, upload['target']) 164​ gfs.upload(tmpfname, upload['target'])
165​ os.remove(tmpfname)
164 # we're all done! rename to the correct name 166​ # we're all done! rename to the correct name
165 os.rename(tmpfile, self.filename) 167​ os.rename(tmpfile, self.filename)
166 except: 168​ except:
167 # if anything went wrong, we want to wipe the temp file 169​ # if anything went wrong, we want to wipe the temp file
168 # then raise 170​ # then raise
169 os.remove(tmpfile) 171​ os.remove(tmpfile)
170 raise 172​ raise
171 finally: 173​ finally:
172 # whether things go right or wrong, we want to close the 174​ # whether things go right or wrong, we want to close the
173 # gfs instance 175​ # gfs instance, and rwmj recommends 'shutdown()' too
176​ gfs.shutdown()
174 gfs.close() 177​ gfs.close()
175 178​
176 179​
177class VirtBuilderImage(object): 180​class VirtBuilderImage(object):
178 """Class representing an image created by virt-builder. 'release' 181​ """Class representing an image created by virt-builder. 'release'
179 is the release the image will be built for. 'arch' is the arch. 182​ is the release the image will be built for. 'arch' is the arch.
180 'size' is the desired image size, valid formats are a digit string 183​ 'size' is the desired image size, valid formats are a digit string
181 (size in bytes, digit string plus 'M', 'MB' or 'MiB' (size in 184​ (size in bytes, digit string plus 'M', 'MB' or 'MiB' (size in
▲ Show 20 Lines • Show All 475 Lines • Show Last 20 Lines