[blivet:f21-branch/master 3/3] Make self.formatClass not a field (#1150147)

Vratislav Podzimek vpodzime at redhat.com
Mon Oct 13 14:01:05 UTC 2014


On Mon, 2014-10-13 at 08:14 -0400, Anne Mulhern wrote:
> 
> 
> 
> ----- Original Message -----
> > From: "Vratislav Podzimek" <vpodzime at redhat.com>
> > To: "anaconda patch review" <anaconda-patches at lists.fedorahosted.org>
> > Sent: Monday, October 13, 2014 3:42:50 AM
> > Subject: Re: [blivet:f21-branch/master 3/3] Make self.formatClass not a	field (#1150147)
> > 
> > On Fri, 2014-10-10 at 12:24 -0400, mulhern wrote:
> > > Related: fed#1150147
> > > 
> > > Remove constructor, because setting formatClass was all it did before
> > > calling superconstructor.
> > > 
> > > self.formatClass field is only used in one method,
> > > so might as well make it local to that one method as set it in constructor.
> > > 
> > > In constructor, it is necessary to set if before superclass constructor
> > > call, which is a sign that something is slightly wrong (as superclass
> > > state depends on additional subclass fields). Ideally, we could
> > > be in a situation where we always call superclass constructor at start
> > > of subclass constructor. In reality, that's not happening, but it
> > > can here, so that's another plus.
> > One big difference is that doing the call in the constructor means the
> > exception is raised at that time. Is it okay to gracefully create a
> > device with a wrong format and rthen raise an exception later when it is
> > used?
> 
> There is no creating a device with any format that occurs in the ContainerDevice constructor.
> What is calculated is the format that that devices requires its parents to posses.
> 
> This format is only used in the situation where a parent is added, in the current code,
> and I'm not aware of any plans to use it in any other situation, although there might be.
> 
> There are many instances in blivet code where a method raises an exception even though
> the constructor did not. This would just be another, except that it is possible to
> demonstrate that it will never happen, by the following argument:
> 
> _addParent() will always be invoked by superclass constructor OR the number of parents
> will be 0, which will cause an exception to be raised in the constructor.
> 
> However, this patch got withheld anyway, by the argument that at some
> point, _addParent() might no longer check that the format was correct, and therefore not
> need to invoke the code to find that format that it requires its members to possess,
> and that it would still somehow be desirable to have a field that holds that format
> (perhaps because at that time it was used in some other code that checked parents'
> format type, maybe by some anaconda sanityCheck() code that needed access to that
> information, or something like that).
> 
> In any case, this irritating bug brought to light again the fact that
> "mdcontainer" devices have to be special cased all over, and that the
> real solution is to split these out into a class that is separate
> from MDRaidArrayDevice, or maybe a subtype of it. So really, the lighter
> the patch set that deals with this current problem, the better,
> assuming that that task is accomplished in the reasonably near future.
Cannot agree more.

-- 
Vratislav Podzimek

Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic



More information about the anaconda-patches mailing list