Change in vdsm[master]: Add partial vms list support to the regular 'list' query.

danken at redhat.com danken at redhat.com
Tue Sep 20 07:41:20 UTC 2011


Dan Kenigsberg has posted comments on this change.

Change subject: Add partial vms list support to the regular 'list' query.
......................................................................


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

(5 inline comments)

....................................................
File vdsm/clientIF.py
Line 685:         except:
Line 686:             self.log.debug(traceback.format_exc())
Line 687:             return errCode['unexpected']
Line 688: 
Line 689:     def list(self, vms=None, full=True):
please add new args at the end, for backward compat.

vms=[] makes more sense to me.
Line 690:         """ return a list of known VMs with full (or partial) config each """
Line 691:         def reportedStatus(vm, full):
Line 692:             d = vm.status()
Line 693:             if full:


Line 695
Line 696
Line 697
Line 698
Line 699
I'd simply add

if not vms or vm.id in vms

into the list comprehension
Line 700
Line 701
Line 702
Line 703


....................................................
File vdsm_cli/vdsClient.py
Line 192:         return self.ExecAndExit(self.s.changeFloppy(vmId, file))
Line 193: 
Line 194:     def do_list(self, args):
Line 195:         """
Line 196:         Usage: vdsClient 0 list [vms:vmId1,vmId2] [table/long/ids]
it would be more polite to maintain arg order.
Line 197:         """
Line 198:         vmListViews = ('table', 'long', 'ids')
Line 199:         view = 'long' #Default view
Line 200:         vms = []


Line 199:         view = 'long' #Default view
Line 200:         vms = []
Line 201: 
Line 202:         if args:
Line 203:             if args[0].startswith('vms:'):
it is not your fault... but vdsClient args are pure hell.
do we ever use this kind of arg qualifiers?
Line 204:                 vms = args[0].split(':')[1].strip()
Line 205:                 if vms:
Line 206:                    vms = [vm.strip() for vm in vms.split(',')]
Line 207:                 else:


Line 218:                 if not vms:
Line 219:                    for res in self.s.getAllVmStats()['statsList']:
Line 220:                          allStats[ res['vmId'] ] = res
Line 221:                 else:
Line 222:                    for vm in vms:
why bother the server so much? I'd use the single getAll... call, but fill in only relevant vms.
Line 223:                        res = self.s.getVmStats(vm)['statsList']
Line 224:                        allStats[vm] = res[0]
Line 225: 
Line 226:         response = self.s.list(vms, True)


--
To view, visit http://gerrit.usersys.redhat.com/956
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5051c42ca9e4ce706eff3cfec0de04cd76b2b95f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Igor Lvovsky <ilvovsky at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky at redhat.com>


More information about the vdsm-patches mailing list