Bug#335230: Random Code so far to be cleaned
Thanks; that was fast.
In the following comment, there are occasional notes "so far we did
it differently". That does not necessarily mean your approach is wrong,
however I would prefer not to introduce variations in style if we don't
have to. The unity in style should make it easier for future generations
to familiarize themselves with the code ...
In other situations, I found it effective to first build analysis code
that is invoked with the --test option, that just shows which evms devices
are available and what are their underlying devices.
Note that I haven't played with EVMS yet; these are mostly issues of fitting
in with the existing code, not test results.
On Mon, Oct 24, 2005 at 06:33:42PM +0200, Marco Amadori wrote:
> diff -urN yaird-0.0.11/perl/Evms.pm yaird-0.0.11-evms/perl/Evms.pm
> --- yaird-0.0.11/perl/Evms.pm 1970-01-01 01:00:00.000000000 +0100
> +++ yaird-0.0.11-evms/perl/Evms.pm 2005-10-24 18:31:24.000000000 +0200
> @@ -0,0 +1,70 @@
> +#!perl -w
> +#
> +# Evms -- encapsulate evms_gather output
> +# Copyright (C) 2005 Erik van Konijnenburg, Marco Amadori
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> +#
> +
> +use strict;
> +use warnings;
> +use Base;
> +use Conf;
Conf is probably redundant
> +use base 'Obj';
> +package Evms;
These two lines may need to be swapped.
> +
> +sub fill {
> + my $self = shift;
> + $self->SUPER::fill();
> + $self->takeArgs ('path', 'devno', 'disks', 'plugins', 'version');
> +}
> +
> +sub path { return $_[0]->{path}; }
> +sub devno { return $_[0]->{devno}; } # 4 what?
> +sub disks { return $_[0]->{disks}; }
ok ...
> +sub plugins { return $_[0]->{plugins}; } # not implemented
> +sub version { return $_[0]->{version}; }
can these differ between devices in a single system?
if not, not much sense in making them attributes of individual disks..
> +
> +sub string {
> + my $self = shift;
> + my $path = $self->path;
> + my $devno = $self->devno;
> + my $disks = join (',', @{$self->disks});
> + my $plugins = join (',', @{$self->plugins});
> + return "$path($devno) = $plugins on $disks";
> +}
> +
> +
> +sub findDisksByName ($) {
So far, the find functions have been kept in a module
named like YyyTab.pm, separate from the YyyDev.pm, the YyyDev
then contains just access functions like string() and
friends.
perhaps we need an EvmsTab.pm?
> +# should provide a list from evms_query disks <name>
> +# like the output of the shell command
> +# evms_query disks /dev/evms/lvm2/safe/usr64 | xargs -i echo /dev/\{\}
> +# /dev/sda
> +# /dev/sdb
> +# /dev/sdc
> + my ($name) = @_;
> +
> + return $evms->disks;
$evms? from where? So far, a module EvmsTab contained a variable
$evmsTab. A find() function then does init(), and init() will initialise
$evmsTab if it is not yet defined; doing so by parsing output of evms_query.
The find() function can walk $evmsTab afterwards.
Note that this is easier to integrate if you have a findByDevno()
that returns an Evms object given a major:minor for a device, or undef.
> +
> +
> +sub findVersion () {
> +# equivalent of evms_query info | head -1 | cut -d ' ' -f 3
> +# e.g. from the output "EVMS Version: 2.5.3 \
> +# EVMS Engine API Version: 10.1.0" gives "2.5.3"
> +
> + return $evms->version;
> +}
> +
> +1;
> diff -urN yaird-0.0.11/perl/Plan.pm yaird-0.0.11-evms/perl/Plan.pm
> --- yaird-0.0.11/perl/Plan.pm 2005-08-07 22:57:40.000000000 +0200
> +++ yaird-0.0.11-evms/perl/Plan.pm 2005-10-24 18:28:12.375146768 +0200
> @@ -1,7 +1,7 @@
> #!perl -w
> #
> # Plan -- high-level stuff
> -# Copyright (C) 2005 Erik van Konijnenburg
> +# Copyright (C) 2005 Erik van Konijnenburg, Marco Amadori
> #
> # This program is free software; you can redistribute it and/or modify
> # it under the terms of the GNU General Public License as published by
> @@ -25,6 +25,7 @@
> use FsTab;
> use ActiveBlockDevTab;
> use LvmTab;
> +use Evms;
> use Hardware;
> use ModProbe;
> use RaidTab;
> @@ -83,6 +84,7 @@
> }
>
> my $ok = 0;
> + $ok || ($ok = tryEvms ($actions,$device,[$device,@{$working}]));
Tricky. Trying Evms even before verifying a device may be a partition
of other device? The partition check is simple, cheap and foolproof;
a good one to start with.
> $ok || ($ok = tryParent ($actions,$device,[$device,@{$working}]));
> $ok || ($ok = tryDmCrypt ($actions,$device,[$device,@{$working}]));
> $ok || ($ok = tryLvm ($actions,$device,[$device,@{$working}]));
> @@ -93,6 +95,35 @@
> }
> Base::debug ("device: completed $name");
> }
> +#
> +# tryEvms --
> +#
> +
> +sub tryEvms ($$$) {
> + my ($actions, $device, $working) = @_;
> +
> + my $name = $device->name; # If name is like /dev/evms/root, if is like dm-3
> I'm lost
I don't understand the comment: so far all device->name have been
kernel names, your next check also assumes that.
> + if ($name !~ /^dm-\d+$/) {
> + return 0;
> + }
> +
At this point I would expect
my $evmsDevice = EvmsTab::findByDevno($device->devno)
where someone else already determined what device kernel-name and devno go
together. Note how Base::devno can return a major:minor if you have the pathname
of a block special file.
The findByDevno could return undef if no evms for this device exists;
which is not fatal, just return 0, since a dm-\d device could also be dm-crypt
or lvm.
The nice bit is that if /etc/fstab mounts /dev/peanutbutter, and evms uses
/dev/emvs/chunky-bacon, yaird will know both files are the same as long as
both use the same device number.
> + for my $evmsDisk (@{Evms::findDisksByName($name)})
> + {
One would expect @{$evmsDevice->disks}, the disks underlying this evms
device.
> + my $pdev = ActiveBlockDevTab::findByPath ($evmsDisk);
Yes, if you represent underlying devices with pathnames. An alternative is devno's.
> + my $rc = tryHardware ($actions, $pdev, $working);
Prefered is:
addDevicePlan ($actions, $pdev, $working);
this should also work if the devices underlying the evms are partitions
or crypted or raid or whatever; tryHardware only works for block devices
with a link from /sys/block/blabla0/device to /sys/devices.
> + if ($rc !~ 1) {
> + # Base::fatal ("Can't find EVMS info for $name");
> + return 0;
> + }
AddDevicePlan can fatal(), but cannot return an error.
> +
> + }
> + my $version = Evms::findVersion ()
> + ModProbe::addModules ($actions, [ "dm-mod" ]);
> + $actions->add ("evms_activate", $version);
Note that add ("aap", "noot") will invoke template "aap" with
a template variable that has name 'target' and value "noot".
Note also that the template receives a builtin variable $version,
which is the yaird version.
> +
> + return 1;
> +}
> +
>
> #
> # tryParent -- If it's a partition, do the whole
> diff -urN yaird-0.0.11/templates/Debian.cfg
> yaird-0.0.11-evms/templates/Debian.cfg
> --- yaird-0.0.11/templates/Debian.cfg 2005-08-07 22:57:40.000000000 +0200
> +++ yaird-0.0.11-evms/templates/Debian.cfg 2005-10-24 17:57:31.434012360 +0200
> @@ -250,6 +250,23 @@
> !/sbin/vgchange -a y '<TMPL_VAR NAME=target>'
> END SCRIPT
> END TEMPLATE
> +
> + TEMPLATE evms_activate
> + BEGIN
> + DIRECTORY "/lib/evms/<TMPL_VAR NAME=version>" # Instead of this two lines
> + FILE "/lib/evms/<TMPL_VAR NAME=version>/*.so" # should be used
> addTreeLibrary or similar
You probably want <TMPL_VAR NAME=target>; see above.
You probably want TREE "/lib/evms/<TMPL_VAR NAME=target>";
this expands the template var, it recurses into the tree,
it knows about symlinks, it understands about shared libraries.
But it does not expand wildcards; since it seems *.so is the same as * here,
you could get away with simply copying the whole directory.
> +
> + FILE "/sbin/evms_activate"
> + FILE "/etc/evms.conf"
> + SCRIPT "/init"
> + BEGIN
> + !if [ ! -c /dev/evms ]
> + !then
> + ! /bin/mkdir /dev/evms
> + !fi
> + !/sbin/evms_activate
> + END SCRIPT
> + END TEMPLATE
>
> #
> # NOTE: mdadm can operate without knowledge
Regards,
Erik
Reply to: