Change in vdsm[master]: virt: Don't expose GuestAgent.guestInfo directly

fromani at redhat.com fromani at redhat.com
Thu Jan 7 13:18:28 UTC 2016


Francesco Romani has posted comments on this change.

Change subject: virt: Don't expose GuestAgent.guestInfo directly
......................................................................


Patch Set 1: Code-Review-1

(4 comments)

conceptually fine, but the commit message should be made clearer, and we must be a bit more precise about the copy() tool we want to use.
-1 for visibility only.

https://gerrit.ovirt.org/#/c/51508/1//COMMIT_MSG
Commit Message:

Line 5: CommitDate: 2016-01-07 13:54:38 +0100
Line 6: 
Line 7: virt: Don't expose GuestAgent.guestInfo directly
Line 8: 
Line 9: GuestInfo.getGuestInfo returns its guestInfo dict attribute directly, so
GuestAgent.getGuestInfo() ?
Line 10: the callers may modify it.  guestInfo is a public attribute so it
Line 11: shouldn't be a problem in theory as it's possible to modify that
Line 12: attribute without calling getGuestInfo.  But guestInfo isn't used
Line 13: anywhere outside GuestAgent and should be probably changed to a private


Line 10: the callers may modify it.  guestInfo is a public attribute so it
Line 11: shouldn't be a problem in theory as it's possible to modify that
Line 12: attribute without calling getGuestInfo.  But guestInfo isn't used
Line 13: anywhere outside GuestAgent and should be probably changed to a private
Line 14: attribute.
true, stuff for a future patch.
Line 15: 
Line 16: If we offer guestInfo for modification outside GuestInfo then it may be
Line 17: indeed modified, perhaps without realizing about modification of the
Line 18: internal GuestInfo structure.  That actually already happens in


Line 12: attribute without calling getGuestInfo.  But guestInfo isn't used
Line 13: anywhere outside GuestAgent and should be probably changed to a private
Line 14: attribute.
Line 15: 
Line 16: If we offer guestInfo for modification outside GuestInfo then it may be
outside GuestAgent?
Line 17: indeed modified, perhaps without realizing about modification of the
Line 18: internal GuestInfo structure.  That actually already happens in
Line 19: Vm._getGuestStats and causes incorrect memUsage value reported in case
Line 20: of repeated invocations.


https://gerrit.ovirt.org/#/c/51508/1/vdsm/virt/guestagent.py
File vdsm/virt/guestagent.py:

Line 357:         return self.guestStatus
Line 358: 
Line 359:     def getGuestInfo(self):
Line 360:         if self.isResponsive():
Line 361:             return copy.copy(self.guestInfo)
is a shallow copy sufficient? if so let's have a comment stating this. Or, maybe better, let's just use utils.picklecopy() like everywhere else.
Line 362:         else:
Line 363:             return {
Line 364:                 'username': 'Unknown',
Line 365:                 'session': 'Unknown',


-- 
To view, visit https://gerrit.ovirt.org/51508
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie48c697300e4feeb513dc662c2d1d3d8c21f0af9
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal <mzamazal at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list