[anaconda:master 0/2] RAID patches

Anne Mulhern amulhern at redhat.com
Thu Jun 5 21:41:24 UTC 2014





----- Original Message -----
> From: "Vratislav Podzimek" <vpodzime at redhat.com>
> To: "anaconda patch review" <anaconda-patches at lists.fedorahosted.org>
> Sent: Thursday, June 5, 2014 3:28:22 AM
> Subject: Re: [anaconda:master 0/2] RAID patches
> 
> On Mon, 2014-06-02 at 16:35 -0400, mulhern wrote:
> > These will require the changes in the corresponding blivet RAID patches,
> > so I plan to bump the version number.
> > 
> > The first is big, the second is much smaller. If I could actually profile
> > I'ld be happier, but I believe that the speedup is noticeable to a human.
> > 
> > The only thing that bothers me is this sequence (which was not introduced
> > by these patches):
> > * For, say /, with LVM seleected, press Modify and choose a raid level that
> > BTRFS and LVM are supposed to share, e.g., RAID0. Save the result.
> > * Now, change device type to BTRFS, press modify again, and notice that
> > the RAID level selection is RAID0. Press Cancel or Save.
> > * Change device type back to LVM, press Modify, again, and the selected
> > RAID level is now None, not RAID0.
> > * Go back to BTRFS and notice that the selected RAID level is now 'single',
> > BTRFS's default.
> > 
> > I think that the simple thing to do here is to make sure that, whenever the
> > device type is changed to a new one, the RAID level is set back to the
> > default
> > for that device type. So, in step (2) above, the selected raid level should
> > be the default for BTRFS, "single".
> > 
> > For step (2), whether Cancel or Save is pressed should matter. If Cancel,
> > then, on step(3), the previously selected RAID level, RAID0, should be
> > observed. If Save, then, on Step (3), the default RAID level for LVM,
> > "None",
> > should be observed.
> > 
> > Note that getting the raid level for an LVM volume group seems to mostly
> > get
> > None, and this may be the source of the problem. It will always be None,
> > unless the volume group has exactly one physical volume, because that's
> > what
> > get_raid_level() does.
> > 
> > None of this was introduced by my changes here. But I think it would
> > probably
> > be best to fix it, and also make sure that it is not sign of a deeper
> > problem.
> > 
> > Feedback appreciated, thanks.
> > 
> > mulhern (2):
> >   RAID related changes for custom spoke.
> >   Memoize the results for *RaidLevelsSupported() functions
> > 
> >  pyanaconda/storage_utils.py                        |  17 ---
> >  pyanaconda/ui/gui/spokes/custom.glade              |   4 -
> >  pyanaconda/ui/gui/spokes/custom.py                 |  93 ++++++------
> >  .../ui/gui/spokes/lib/custom_storage_helpers.glade |   4 +
> >  .../ui/gui/spokes/lib/custom_storage_helpers.py    | 163
> >  ++++++++++++++++++---
> >  5 files changed, 189 insertions(+), 92 deletions(-)
> I don't like one thing on these patches -- adding new camelCase
> functions/methods next to the underscore_named ones. I know these two
> styles are mixed in many places, but I'd be really glad if we stuck
> with underscore_based names for new functions/methods where possible.
> 
> Otherwise these look good to me apart from the one comment to PATCH 2/2.
> 
> --
> Vratislav Podzimek
> 
> Anaconda Rider | RHCE | Red Hat, Inc. | Brno - Czech Republic
> 
> 
> _______________________________________________
> anaconda-patches mailing list
> anaconda-patches at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
> 

I'm not particularly attached to any capitalization convention.
I just try to match the surrounding code as best I can.

Is it really the case that C style method names are the standard
for anaconda?

If so, I am on board with that.

- mulhern


More information about the anaconda-patches mailing list