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