'sameas:<device>' broken, fix included

[Thomas Neumann]

On Wednesday 25 June 2014 12:12:15 Thomas Lange wrote:
>> However there's an underlying problem by treating 'disk#' and
>> hda/sda/xvda differently. patch3 addresses that issue.
 
> Mmm, I did a diff on the code block after sameas:(\S+) and
> sameas:disk(\d+). Except for the clone vs. dclone call, it's the same
> code! So why do you see a problem by treating disk# and
> sda,.... differently, when there's no diff in the code?

Because that is _exactly_ the problem. What if someone finds another bug in one of the 2 code paths or wants to change some functionality and forgets to change the other location?

I assume this is exactly what happened with dclone() in 'sameas:disk(\d+)':
The feature was implemented, someone found out it doesn't work when trying 'sameas:disk#', identified the fault, applied the fix ..... and forgot to test / update the other location.

By rewriting the code to use the same code path for both cases it is guaranteed something like this can not happen again in the future.
 
>> If you like the change to be as non-invasive as possible, then you
>> could go
>> for something like this
>> 1) modify | /^sameas:(\S+)/
>> 2) and delete
>> 
>> | /^sameas:disk(\d+)/
 
> That sounds good. And it's a much smaller patch than your patch3.

I'm very sure patch3 would result in easier maintainable code. In fact the current code is actively misleading.

Please look at the following code and decide whether
  my $ref_dev = &FAI::resolve_disk_shortname($1);
and
  my $disk = $1;
refer to the same variable '$1':

----------------------------------------------------------------------------
| /^sameas:disk(\d+)/
{
  my $ref_dev = &FAI::resolve_disk_shortname($1);
  defined($FAI::configs{"PHY_" . $ref_dev}) or die "Reference device $ref_dev not found in config\n";

  $FAI::configs{$FAI::device} = Storable::dclone($FAI::configs{"PHY_" . $ref_dev});
  # add entries to device tree
  defined($FAI::dev_children{$ref_dev}) or
    &FAI::internal_error("dev_children missing reference entry");
  ($FAI::device =~ /^PHY_(.+)$/) or
  &FAI::internal_error("unexpected device name");
  my $disk = $1;
  foreach my $p (@{ $FAI::dev_children{$ref_dev} }) {
    my ($i_p_d, $rd, $pd) = &FAI::phys_dev($p);
      (1 == $i_p_d) or next;
      ($rd eq $ref_dev) or &FAI::internal_error("dev_children is inconsistent");
      push @{ $FAI::dev_children{$disk} }, &FAI::make_device_name($disk, $pd);
  }
}
----------------------------------------------------------------------------

The correct answer is: No, they don't.
Let's try again:

----------------------------------------------------------------------------
| /^sameas:disk(\d+)/
{
  my $ref_dev = &FAI::resolve_disk_shortname($1);
  defined($FAI::configs{"PHY_" . $ref_dev}) or die "Reference device $ref_dev not found in config\n";

  $FAI::configs{$FAI::device} = Storable::dclone($FAI::configs{"PHY_" . $ref_dev});
  # add entries to device tree
  defined($FAI::dev_children{$ref_dev}) or
    &FAI::internal_error("dev_children missing reference entry");
  if ($FAI::device =~ /^PHY_(.+)$/) {
    my $disk = $1;
    foreach my $p (@{ $FAI::dev_children{$ref_dev} }) {
      my ($i_p_d, $rd, $pd) = &FAI::phys_dev($p);
         (1 == $i_p_d) or next;
        ($rd eq $ref_dev) or &FAI::internal_error("dev_children is inconsistent");
        push @{ $FAI::dev_children{$disk} }, &FAI::make_device_name($disk, $pd);
    }
  }
  else {
    &FAI::internal_error("unexpected device name");
  }
}
----------------------------------------------------------------------------

The rest of the patch follows the same pattern - make it obvious what's going on by
properly naming / assigning variables, modifying indentation and stripping unnecessary
stuff.