Vinzenz Feenstra has posted comments on this change.

Change subject: Added a support for a guest hibernate command.
......................................................................


Patch Set 9: I would prefer that you didn't submit this

(3 inline comments)

....................................................
File vdsm/API.py
Line 347:         """
Line 348:         Hibernate a VM.
Line 349: 
Line 350:         :param target: Either "disk", "mem" or an opaque string, 
indicating
Line 351:             the location of hibernation images.
I would mention here also HibernateTarget.DISK and HibernateTarget.MEM as 
values and not the plain strings. If we for whatever reason later decide to 
rename them we might break something else which relied on this documentation.
Line 352:         """
Line 353:         if target == HibernateTarget.DISK or target == 
HibernateTarget.MEM:
Line 354:             try:
Line 355:                 vm = self._cif.vmContainer[self._UUID]


....................................................
File vdsm_cli/vdsClient.py
Line 60: LEAF_VOL = 8
Line 61: 
Line 62: # Guest Hibernate
Line 63: HIBERNATE_TARGET_DISK    = 'disk'
Line 64: HIBERNATE_TARGET_MEMORY  = 'mem'
I would prefer if we could move out such constants into some module which is 
commonly used between VDSM and vdsmCli
Line 65: HIBERNATE_TARGET_DEFAULT = HIBERNATE_TARGET_DISK
Line 66: 
Line 67: def validateArgTypes(args, conv, requiredArgsNumber=0):
Line 68:     if len(args) > len(conv) or len(args) < requiredArgsNumber:


....................................................
File vdsm/vm.py
Line 928:                 self._acpiShutdown()
Line 929:             # No tools, no ACPI
Line 930:             else:
Line 931:                 return {'status': {'code': 
errCode['exist']['status']['code'],
Line 932:                         'message': 'VM without ACPI or active oVirt 
tools. Try forced shutdown.'}}
Should we really have the string 'oVirt' in here?
Line 933:         except:
Line 934:             self.log.error("Shutdown failed", exc_info=True)
Line 935:         return {'status': {'code': doneCode['code'],
Line 936:                 'message': 'Machine shut down'}}


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1ad6836e61e9d91ec6f46a599541f61ff12e9737
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Gal Hammer <[email protected]>
Gerrit-Reviewer: Adam Litke <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Barak Azulay <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Gal Hammer <[email protected]>
Gerrit-Reviewer: Haim Ateya <[email protected]>
Gerrit-Reviewer: Michal Skrivanek <[email protected]>
Gerrit-Reviewer: Vinzenz Feenstra <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Reply via email to