Francesco Romani has posted comments on this change. Change subject: vmdisk hook: add support for booting from image file ......................................................................
Patch Set 6: Code-Review+1 (6 comments) nice addition. *IF* you wish, I added comments about possible improvements, but the patch looks good enough, so they can be addressed later. https://gerrit.ovirt.org/#/c/58748/6/vdsm_hooks/vmdisk/README File vdsm_hooks/vmdisk/README: PS6, Line 3: virtio-blk well, this seems too specific. What prevents your change to work also with other types of disks (e.g. virtio) ? PS6, Line 3: ordered after regular VM disks I don't understand which boot order are you looking for. You want this hook to make the disk boot after every other disk, but before any network interface (e.g. PXE)? https://gerrit.ovirt.org/#/c/58748/6/vdsm_hooks/vmdisk/before_vm_start.py File vdsm_hooks/vmdisk/before_vm_start.py: PS6, Line 70: [] you may want to use a set() here PS6, Line 73: if len(btag) > 0: if btag: # ... should be enough PS6, Line 84: bootorders using sorted(bootorders) make it clearer (and a bit safer i'd say) PS6, Line 83: for i in range(1, 28): : if not str(i) in bootorders: : boot.setAttribute('order', str(i)) : break not sure this what you I understand you want from the README additions. This takes the first unused number, so nothing makes it go before the network interfaces. -- To view, visit https://gerrit.ovirt.org/58748 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib9b3f6239f3c58a9f9497e74e5d9c8993d78d81e Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Dmitry Glushenok <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Dmitry Glushenok <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Milan Zamazal <[email protected]> Gerrit-Reviewer: Yaniv Kaul <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
