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