CheckedIPAddress: Implement __(g|s)etstate__ and to ensure proper (un)pickling #142
Conversation
|
IMO here (init of CheckedIPAddress) is missing self._net = addr._net it may cause issues and self._net should be handled in parent class |
|
Works for me, I'm not pushing this yet if somebody wants to double check pickle implementation |
| state['super_state'] = super(UnsafeIPAddress, self).__getstate__() | ||
| except AttributeError: | ||
| # none of base classes implements custom pickling | ||
| pass |
There was a problem hiding this comment.
We know IPAddress implements custom pickling, so the try block is unnecessary. Actually, if custom pickling was removed from IPAddress, the try block would break pickling completely, so it's not unnecessary, but harmful. Please remove it.
| @@ -127,12 +127,34 @@ def __init__(self, addr): | |||
| super(UnsafeIPAddress, self).__init__(addr, | |||
| flags=self.netaddr_ip_flags) | |||
|
|
|||
| def __getstate__(self): | |||
| state = {attr: getattr(self, attr) for attr in ['_net']} | |||
There was a problem hiding this comment.
IMHO state = {'_net': self._net} is all you need, there is no benefit in dict comprehension.
| else: | ||
| super(UnsafeIPAddress, self).__setstate__(super_state) | ||
|
|
||
| self.__dict__.update(state) |
There was a problem hiding this comment.
You should be explicit about what you are unpickling, not only because explicit is better than implicit, but also for robustness - you can't guarantee the pickled data will contain only the attributes you want.
|
NACK, see inline comments. |
| @@ -142,6 +153,7 @@ def __init__(self, addr, match_local=False, parse_netmask=True, | |||
|
|
|||
| if isinstance(addr, CheckedIPAddress): | |||
| self.prefixlen = addr.prefixlen | |||
| self._net = addr._net | |||
There was a problem hiding this comment.
self._net is already set by the super(CheckedIPAddress, self).__init__(addr) call above.
There was a problem hiding this comment.
Yes, but in init of CheckedIPAddress, there is custom initialization of self._net, so creating new instance using copying might get different values (None) in new object by using just super call without explicit variable assigment.
There was a problem hiding this comment.
Given addr is an instance of CheckedIPAddress, when this is called from CheckedIPAddress.__init__:
super(CheckedIPAddress, self).__init__(addr)it will call this bit (and only this bit) of UnsafeIPAddress.__init__:
if isinstance(addr, UnsafeIPAddress):
self._net = addr._net
super(UnsafeIPAddress, self).__init__(addr,
flags=self.netaddr_ip_flags)
returnwhich uncoditionally copies the value of addr._net to self._net, so how exactly would self._net end up as None when addr._net is not None too?
There was a problem hiding this comment.
Ah sorry, I missed that.
| return state | ||
|
|
||
| def __setstate__(self, state): | ||
| super(UnsafeIPAddress, self).__setstate__(state.pop('super_state')) |
There was a problem hiding this comment.
You don't really have to .pop() anymore, as state is no longer passed further.
…pickling Missing attributes in instance created by pickle.load cause AttributeError in second part of ipa-server-install --external-ca. https://fedorahosted.org/freeipa/ticket/6385
Missing attributes in instance created by pickle.load cause AttributeError in
second part of ipa-server-install --external-ca.
https://fedorahosted.org/freeipa/ticket/6385