setup_harddisks_2 bug

[Paul Lussier]

Hi,

I've been playing around with setting up raid using FAI (v. 2.8.4)
with the setup_harddisks 2 script from:

   http://faiwiki.informatik.uni-koeln.de/index.php/Setup_harddisks_2

in which I think I've found a bug where sfdisk bails out
immediately upon not being able to query a non-existant drive.
On line 480 of setup_harddisks is the following line:

    $result = `sh -c "LC_ALL=C sfdisk /dev/sd[a-z] /dev/hd[a-z] -g -q" 2>&1`;

If this is run on a system with *only* IDE PATA drives, which use
the /dev/hdX nomenclature, sfdisk will fail to return a list of
drives in the system because it first fails when it encounters a
lack of /dev/sdX drives.

A better way to do this would be to split this into 2 commands:

    $result = `sh -c "LC_ALL=C sfdisk /dev/sd[a-z] -g -q" 2>&1`;
    $result .= `sh -c "LC_ALL=C sfdisk /dev/hd[a-z] -g -q" 2>&1`;

Of course, this really isn't optimal either, since in both cases
setup_harddisks is attempting to query a list of drives which in
all likeliness do not exist.  For example, what are the chances
that system contains /dev/sdz or /dev/hdz?  Could it?  Sure, but
why not find out what devices actually exist and probe them
directly and explicitly rather than trying blindly probing every
possibility and failing?

I suggest an approach similar to the following:

1. Create a subroutine to obtain a list of disks in the system:

  ######################################################################
  #
  # Return a list of disks in the system
  #
  # @returns  a list of disks in the system
  ##
  sub getDisks {
    my @stdout = split(/\n/,`sfdisk -s`);
    my @disks = grep { !/md/ } @stdout;
    @disks = sort grep(/dev/, at disks);
    map { s/(.*):.*/$1/ } @disks;
    wantarray ? return @disks : return \@disks;
  }

2. Create a subroutine to probe *A* disk:

  ######################################################################
  #
  # Return the geometry of a disk. 
  # @param   $drive  A drive to probe with sfdisk
  #
  # @returns  a hash containing the geometry of the drive passed in
  ##
  sub getDriveGeometry {
    my $drive = shift;
    my $geometry = {};
    my $stdout = `sh -c "LC_ALL=C sfdisk $drive -g -q" 2>&1`;
    chomp($stdout);

    if ($stdout && $stdout ne "") {
      m{^/dev/.+?:\s+
      (\d+)\s+cylinders,\s+
      (\d+)\s+heads,\s+
      (\d+)\s+sectors }ix;
      $geometry = (cylinders => $1,
                   heads => $2,
                            sectors => $3,
                                       cylinderSize => $heads * $sectors,
                                                      );
    }
    retun $geometry;
  }

3. Call these routines from the GetAllDisks():

  my %drives;
  @drives = getDisks();
  foreach my $disk (@drives) {
    $drives{$disk} = getDriveGeometry($disk);
  }

Another suggestion for this same part of the code is to clear out
$result in between the assignments to it and check that there is
actually a valid value getting assigned to it.  $result could contain
the empty string at any point and there's no checking of that, nor any
error handling if that's the case.  The code just goes right ahead and
uses it's value regardless of what that value is assuming it's
correct.  This can make for extremely troublesome debugging.

I hope this helpful criticism.

Thanks.
-- 
Seeya,
Paul
--
Key fingerprint = 1660 FECC 5D21 D286 F853  E808 BB07 9239 53F1 28EE

A: Yes.                                                               
> Q: Are you sure?                                                    
>> A: Because it reverses the logical flow of conversation.           
>>> Q: Why is top posting annoying in email?