[PATCH 6/9] Add action classes for container member set management.

Anne Mulhern amulhern at redhat.com
Mon Mar 24 12:08:14 UTC 2014





----- Original Message -----
> From: "David Lehman" <dlehman at redhat.com>
> To: "anaconda patch review" <anaconda-patches at lists.fedorahosted.org>
> Sent: Friday, March 21, 2014 11:14:02 AM
> Subject: Re: [PATCH 6/9] Add action classes for container member set	management.
> 
> On Fri, 2014-03-21 at 08:51 +0100, Vratislav Podzimek wrote:
> > On Thu, 2014-03-20 at 12:14 -0500, David Lehman wrote:
> > > ---
> > >  blivet/deviceaction.py | 184
> > >  ++++++++++++++++++++++++++++++++++++++++++++++---
> > >  tests/action_test.py   |  96 ++++++++++++++++++++++++++
> > >  2 files changed, 271 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/blivet/deviceaction.py b/blivet/deviceaction.py
> > > index d9a6a87..569377a 100644
> > > --- a/blivet/deviceaction.py
> > > +++ b/blivet/deviceaction.py
> > > @@ -1,8 +1,7 @@
> > >  # deviceaction.py
> > > -# Device modification action classes for anaconda's storage
> > > configuration
> > > -# module.
> > > +# Device modification action classes.
> > >  #
> > > -# Copyright (C) 2009  Red Hat, Inc.
> > > +# Copyright (C) 2009-2014  Red Hat, Inc.
> > >  #
> > >  # This copyrighted material is made available to anyone wishing to use,
> > >  # modify, copy, or redistribute it subject to the terms and conditions
> > >  of
> > > @@ -49,19 +48,25 @@ ACTION_TYPE_NONE = 0
> > >  ACTION_TYPE_DESTROY = 1000
> > >  ACTION_TYPE_RESIZE = 500
> > >  ACTION_TYPE_CREATE = 100
> > > +ACTION_TYPE_ADD = 50
> > > +ACTION_TYPE_REMOVE = 10
> > >  
> > >  action_strings = {ACTION_TYPE_NONE: "None",
> > >                    ACTION_TYPE_DESTROY: "Destroy",
> > >                    ACTION_TYPE_RESIZE: "Resize",
> > > -                  ACTION_TYPE_CREATE: "Create"}
> > > +                  ACTION_TYPE_CREATE: "Create",
> > > +                  ACTION_TYPE_ADD: "Add",
> > > +                  ACTION_TYPE_REMOVE: "Remove"}
> > If we change those dicts to:
> > { key1: val1,
> >   key2: val2,
> >   ...
> >   keyN: valN,
> > }
> > 
> > we will be able to add more items without ruining the git history of the
> > last item. But that's just a neatpick.
> 
> I always disliked that syntax, but I hadn't thought of that reason to
> like/use it.
> 

I tend to prefer not associating the indentation of the keys so closely with the
number of characters in the variable to which they are assigned.
If you need to change action_strings to device_action_strings and you want to keep
indentation that screws up the git history of every key value pair.

Which is better:

action_strings = { ACTION_TYPE_NONE: "None",
   ACTION_TYPE_DESTROY: "Destroy",
   ...etc.
or

action_strings = \
   { ACTION_TYPE_NONE: "None",
     ACTION_TYPE_DESTROY: "Destroy",
     ...etc.

?

I've tended to do the first...but now I'm thinking that the second is better.

- mulhern


> > 
> > >  
> > >  ACTION_OBJECT_NONE = 0
> > >  ACTION_OBJECT_FORMAT = 1
> > >  ACTION_OBJECT_DEVICE = 2
> > > +ACTION_OBJECT_CONTAINER = 3
> > >  
> > >  object_strings = {ACTION_OBJECT_NONE: "None",
> > >                    ACTION_OBJECT_FORMAT: "Format",
> > > -                  ACTION_OBJECT_DEVICE: "Device"}
> > > +                  ACTION_OBJECT_DEVICE: "Device",
> > > +                  ACTION_OBJECT_CONTAINER: "Container"}
> > >  
> > >  RESIZE_SHRINK = 88
> > >  RESIZE_GROW = 89
> > > @@ -148,6 +153,7 @@ class DeviceAction(util.ObjectID):
> > >          if not isinstance(device, StorageDevice):
> > >              raise ValueError("arg 1 must be a StorageDevice instance")
> > >          self.device = device
> > > +        self.container = getattr(self.device, "container", None)
> > >  
> > >      def execute(self):
> > >          """ perform the action """
> > > @@ -178,10 +184,22 @@ class DeviceAction(util.ObjectID):
> > >          return (self.type == ACTION_TYPE_RESIZE and self.dir ==
> > >          RESIZE_GROW)
> > >  
> > >      @property
> > > +    def isAdd(self):
> > > +        return self.type == ACTION_TYPE_ADD
> > > +
> > > +    @property
> > > +    def isRemove(self):
> > > +        return self.type == ACTION_TYPE_REMOVE
> > > +
> > > +    @property
> > >      def isDevice(self):
> > >          return self.obj == ACTION_OBJECT_DEVICE
> > >  
> > >      @property
> > > +    def isContainer(self):
> > > +        return self.obj == ACTION_OBJECT_CONTAINER
> > > +
> > > +    @property
> > >      def isFormat(self):
> > >          return self.obj == ACTION_OBJECT_FORMAT
> > >  
> > > @@ -223,7 +241,7 @@ class DeviceAction(util.ObjectID):
> > >          return _(self.typeDescStr)
> > >  
> > >      def __str__(self):
> > > -        s = "[%d] %s %s" % (self.id, self.typeString, self.objectString)
> > > +        s = "[%d] %s" % (self.id, self.typeDescStr)
> > >          if self.isResize:
> > >              s += " (%s)" % self.resizeString
> > >          if self.isFormat:
> > > @@ -273,6 +291,7 @@ class ActionCreateDevice(DeviceAction):
> > >                  - this action's device depends on the other action's
> > >                  device
> > >                  - both actions are partition create actions on the same
> > >                  disk
> > >                    and this partition has a higher number
> > > +                - the other action adds a member to this device's
> > > container
> > >          """
> > >          rc = False
> > >          if self.device.dependsOn(action.device):
> > > @@ -292,6 +311,9 @@ class ActionCreateDevice(DeviceAction):
> > >                self.device.vg == action.device.vg and
> > >                action.device.singlePV and not self.device.singlePV):
> > >              rc = True
> > > +        elif (action.isAdd and action.container == self.container):
> > > +            rc = True
> > > +
> > >          return rc
> > >  
> > > 
> > > @@ -325,6 +347,7 @@ class ActionDestroyDevice(DeviceAction):
> > >                  - the other action's device depends on this action's
> > >                  device
> > >                  - both actions are partition create actions on the same
> > >                  disk
> > >                    and this partition has a lower number
> > > +                - the other action removes this action's device from a
> > > container
> > >          """
> > >          rc = False
> > >          if action.device.dependsOn(self.device) and action.isDestroy:
> > > @@ -342,6 +365,8 @@ class ActionDestroyDevice(DeviceAction):
> > >                action.device.id == self.device.id):
> > >              # device destruction comes after destruction of device's
> > >              format
> > >              rc = True
> > > +        elif (action.isRemove and action.device == self.device):
> > > +            rc = True
> > >          return rc
> > >  
> > >      def obsoletes(self, action):
> > > @@ -352,6 +377,10 @@ class ActionDestroyDevice(DeviceAction):
> > >  
> > >              - obsoletes all but ActionDestroyFormat actions w/ lower id
> > >              on the
> > >                same device if device exists
> > > +
> > > +            - obsoletes all actions that add a member to this action's
> > > +              (container) device
> > > +
> > >          """
> > >          rc = False
> > >          if action.device.id == self.device.id:
> > > @@ -361,6 +390,10 @@ class ActionDestroyDevice(DeviceAction):
> > >                   self.device.exists and \
> > >                   not (action.isDestroy and action.isFormat):
> > >                  rc = True
> > > +            elif action.isAdd and (action.device == self.device):
> > > +                rc = True
> > > +        elif action.isAdd and (action.container == self.device):
> > > +            rc = True
> > >  
> > >          return rc
> > >  
> > > @@ -413,6 +446,8 @@ class ActionResizeDevice(DeviceAction):
> > >                    this action's device depends on
> > >                  - the other action shrinks a device (or format it
> > >                  contains)
> > >                    that depends on this action's device
> > > +                - the other action removes this action's device from a
> > > container
> > > +                - the other action adds a member to this device's
> > > container
> > >          """
> > >          retval = False
> > >          if action.isResize:
> > > @@ -424,6 +459,10 @@ class ActionResizeDevice(DeviceAction):
> > >                  retval = True
> > >              elif action.isShrink and
> > >              action.device.dependsOn(self.device):
> > >                  retval = True
> > > +        elif (action.isRemove and action.device == self.device):
> > > +            rc = True
> > > +        elif (action.isAdd and action.container == self.container):
> > > +            rc = True
> > These two 'rc's should be 'retval's.
> 
> Fixed throughout.
> 
> > 
> > >  
> > >          return retval
> > >  
> > > @@ -488,12 +527,14 @@ class ActionCreateFormat(DeviceAction):
> > >              Format create action can require another action if:
> > >  
> > >                  - this action's device depends on the other action's
> > >                  device
> > > -                  and the other action is not a device destroy action
> > > +                  and the other action is not a device destroy action or
> > > a
> > > +                  container action
> > >                  - the other action is a create or resize of this
> > >                  action's
> > >                    device
> > >          """
> > >          return ((self.device.dependsOn(action.device) and
> > > -                 not (action.isDestroy and action.isDevice)) or
> > > +                 not ((action.isDestroy and action.isDevice) or
> > > +                      action.isContainer)) or
> > >                  (action.isDevice and (action.isCreate or
> > >                  action.isResize) and
> > >                   self.device.id == action.device.id))
> > >  
> > > @@ -545,8 +586,15 @@ class ActionDestroyFormat(DeviceAction):
> > >  
> > >                  - the other action's device depends on this action's
> > >                  device
> > >                    and the other action is a destroy action
> > > +                - the other action removes this action's device from a
> > > container
> > >          """
> > > -        return action.device.dependsOn(self.device) and action.isDestroy
> > > +        retval = False
> > > +        if action.device.dependsOn(self.device) and action.isDestroy:
> > > +            retval = True
> > > +        elif (action.isRemove and action.device == self.device):
> > > +            rc = True
> > Same here.
> > 
> > > +
> > > +        return retval
> > >  
> > >      def obsoletes(self, action):
> > >          """ Return True if this action obsoletes action.
> > > @@ -611,6 +659,7 @@ class ActionResizeFormat(DeviceAction):
> > >                    that depends on this action's device
> > >                  - the other action grows a device (or format) that this
> > >                    action's device depends on
> > > +                - the other action removes this action's device from a
> > > container
> > >          """
> > >          retval = False
> > >          if action.isResize:
> > > @@ -622,5 +671,122 @@ class ActionResizeFormat(DeviceAction):
> > >                  retval = True
> > >              elif action.isGrow and self.device.dependsOn(action.device):
> > >                  retval = True
> > > +        elif (action.isRemove and action.device == self.device):
> > > +            rc = True
> > And here again.
> > 
> > > +
> > > +        return retval
> > > +
> > > +
> > > +class ActionAddMember(DeviceAction):
> > > +    """ An action representing addition of a member device to a
> > > container. """
> > > +    type = ACTION_TYPE_ADD
> > > +    obj = ACTION_OBJECT_CONTAINER
> > > +    typeDescStr = N_("add container member")
> > > +
> > > +    def __init__(self, container, device):
> > > +        super(ActionAddMember, self).__init__(device)
> > > +        self.container = container
> > > +        container._addMember(device)
> > > +
> > > +    def cancel(self):
> > > +        self.container._removeMember(self.device)
> > > +
> > > +    def execute(self):
> > > +        self.container.add(self.device)
> > > +
> > > +    def requires(self, action):
> > > +        """
> > > +            requires
> > > +                - create/resize the same device
> > > +
> > > +            required by
> > > +                - any create/grow action on a device in the same
> > > container
> > > +        """
> > > +        retval = False
> > > +        if ((action.isCreate or action.isResize) and
> > > +            action.device == self.device):
> > > +            retval = True
> > > +
> > > +        return retval
> > This could be narrowed down to:
> > return ((action.isCreate or action.isResize) and
> >          action.device == self.device)
> 
> Done.
> 
> > 
> > > +
> > > +    def obsoletes(self, action):
> > > +        """
> > > +            obsoletes
> > > +                - remove same member from same container
> > > +                - add same member to same container w/ higher id
> > > +
> > > +            obsoleted by
> > > +                - destroy the container
> > > +                - destroy the device
> > > +                - remove same member from same container
> > > +        """
> > > +        retval = False
> > > +        if (action.isRemove and
> > > +            action.device == self.device and
> > > +            action.container == self.container):
> > > +            retval = True
> > > +        elif (action.isAdd and
> > > +              action.device == self.device and
> > > +              action.container == self.container and
> > > +              action.id > self.id):
> > > +            retval = True
> > > +
> > > +        return retval
> > This could again be a single return line or at least those retval
> > assignments (with the first one dropped) should be replaced with return
> > statements with 'return False' at the end.
> 
> I like having a single return statement in these types of places. It
> makes debugging much easier if the need arises. Is there a significant
> performance degradation in doing it this way?
> 
> > 
> > > +
> > > +
> > > +class ActionRemoveMember(DeviceAction):
> > > +    """ An action representing removal of a member device from a
> > > container. """
> > > +    type = ACTION_TYPE_REMOVE
> > > +    obj = ACTION_OBJECT_CONTAINER
> > > +    typeDescStr = N_("remove container member")
> > > +
> > > +    def __init__(self, container, device):
> > > +        super(ActionRemoveMember, self).__init__(device)
> > > +        self.container = container
> > > +        container._removeMember(device)
> > > +
> > > +    def cancel(self):
> > > +        self.container._addMember(self.device)
> > > +
> > > +    def execute(self):
> > > +        self.container.remove(self.device)
> > > +
> > > +    def requires(self, action):
> > > +        """
> > > +            requires
> > > +                - any destroy/shrink action on a device in the same
> > > container
> > > +                - any add action on this container
> > > +
> > > +            required by
> > > +                - any destroy/resize action on the device
> > > +        """
> > > +        retval = False
> > > +        if ((action.isShrink or action.isDestroy) and
> > > +            action.device.container == self.container):
> > > +            retval = True
> > > +        elif action.isAdd and action.container == self.container:
> > > +            retval = True
> > > +
> > > +        return retval
> > > +
> > > +    def obsoletes(self, action):
> > > +        """
> > > +            obsoletes
> > > +                - add same member to same container
> > > +                - remove same member from same container w/ higher id
> > > +
> > > +            obsoleted by
> > > +                - add same member to same container
> > > +        """
> > > +        retval = False
> > > +        if (action.isAdd and
> > > +            action.device == self.device and
> > > +            action.container == self.container):
> > > +            retval = True
> > > +        elif (action.isRemove and
> > > +              action.device == self.device and
> > > +              action.container == self.container and
> > > +              action.id > self.id):
> > > +            retval = True
> > >  
> > >          return retval
> > Same in those two methods.
> > 
> 
> 
> _______________________________________________
> anaconda-patches mailing list
> anaconda-patches at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
> 


More information about the anaconda-patches mailing list