Change in vdsm[master]: WIP vm payload: cloud-init - multiple files, custom volume l...
danken at redhat.com
danken at redhat.com
Mon Jun 17 21:49:09 UTC 2013
Dan Kenigsberg has posted comments on this change.
Change subject: WIP vm payload: cloud-init - multiple files, custom volume label
......................................................................
Patch Set 1: I would prefer that you didn't submit this
(8 inline comments)
....................................................
File tests/mkimageTests.py
Line 123: """
Line 124: checkSudo(["mount", "-o", "loop", "somefile", "target"])
Line 125: checkSudo(["umount", "target"])
Line 126: iso_img = mkimage.mkIsoFs("vmId_iso", self.files, None)
Line 127: # TODO add test for label, same in test_mkFloppyFs above
Any reason not to do it now, by passing a simple string instead of None? Or even both, with the help of testrunner.permutations().
Line 128: self.assertTrue(os.path.exists(iso_img))
Line 129: m = storage.mount.Mount(iso_img, self.workdir)
Line 130: m.mount(mntOpts='loop')
Line 131: try:
....................................................
File vdsm/clientIF.py
Line 307: except AttributeError:
Line 308: raise vm.VolumeError("Unsupported 'device': %s in "
Line 309: "drive: %" % (drive['device'], drive))
Line 310: else:
Line 311: # the only reason for adding these variables is that
I'd rather drop this comment than maintain it. It's not completely exact (PEP8 won't mind if it's taken into a helper function).
Line 312: # you can not write this without breaking PEP8
Line 313: file_list = drive['specParams']['vmPayload']['file']
Line 314: volId = drive['specParams']['vmPayload'].get('volId', None)
Line 315: volPath = mkFsFunction(vmId, file_list, volId)
Line 309: "drive: %" % (drive['device'], drive))
Line 310: else:
Line 311: # the only reason for adding these variables is that
Line 312: # you can not write this without breaking PEP8
Line 313: file_list = drive['specParams']['vmPayload']['file']
"files" is a better name for a dictionary...
Line 314: volId = drive['specParams']['vmPayload'].get('volId', None)
Line 315: volPath = mkFsFunction(vmId, file_list, volId)
Line 316:
Line 317: elif "path" in drive:
Line 310: else:
Line 311: # the only reason for adding these variables is that
Line 312: # you can not write this without breaking PEP8
Line 313: file_list = drive['specParams']['vmPayload']['file']
Line 314: volId = drive['specParams']['vmPayload'].get('volId', None)
No need to specify None, as it is the default of dict.get().
Line 315: volPath = mkFsFunction(vmId, file_list, volId)
Line 316:
Line 317: elif "path" in drive:
Line 318: volPath = drive['path']
....................................................
File vdsm/mkimage.py
Line 50: filename = os.path.join(parentdir, name)
Line 51: dirname = os.path.dirname(filename)
Line 52: if not os.path.exists(dirname):
Line 53: try:
Line 54: os.makedirs(dirname)
The two users of this function are supposed to have the directory created already. It is not clear why this is needed. If it's a bug fix, please dedicate a separate patch with its own commit message to it.
Line 55: except OSError as e:
Line 56: if e.errno != os.errno.EEXIST:
Line 57: raise
Line 58: with file(filename, 'w') as f:
Line 80: path = os.path.join(_P_PAYLOAD_IMAGES, "%s.%s.img" % (vmId, md5))
Line 81: return path
Line 82:
Line 83:
Line 84: def mkFloppyFs(vmId, files, volId):
In this level, it's better to follow mkfs.msdos naming, and call it volumeName.
Also, it's nice to make it an optional arg with
volumeName=None
Line 85: floppy = dirname = None
Line 86: try:
Line 87: floppy = _getFileName(vmId, files)
Line 88: if volId:
Line 87: floppy = _getFileName(vmId, files)
Line 88: if volId:
Line 89: command = [EXT_MKFS_MSDOS, '-C', floppy, '1440', '-n', volId]
Line 90: else:
Line 91: command = [EXT_MKFS_MSDOS, '-C', floppy, '1440']
having
if volumeName is not None:
command.extend([-n, volumeName])
would avoid copy-and-paste.
Line 92: rc, out, err = storage.misc.execCmd(command, raw=True)
Line 93: if rc:
Line 94: raise OSError(errno.EIO, "could not create floppy file: "
Line 95: "code %s, out %s\nerr %s" % (rc, out, err))
Line 116:
Line 117: if volId:
Line 118: command = [EXT_MKISOFS, '-r', '-o', isopath, '-V', volId, dirname]
Line 119: else:
Line 120: command = [EXT_MKISOFS, '-r', '-o', isopath, dirname]
please use extend() tricks here, too.
Line 121: rc, out, err = storage.misc.execCmd(command, raw=True)
Line 122: if rc:
Line 123: raise OSError(errno.EIO, "could not create iso file: "
Line 124: "code %s, out %s\nerr %s" % (rc, out, err))
--
To view, visit http://gerrit.ovirt.org/14347
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5247d8f4ee01c0c871912a7998bdf958fc71ddd1
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett <gpadgett at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
More information about the vdsm-patches
mailing list