From 0e16a65d80bc92e1f7ea4ec137c6a2dddf26dfd1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Jourdois?= Date: Sun, 18 Jul 2010 16:16:43 +0200 Subject: [PATCH] Validate option arguments Also provide a default value for serial_device and disk_device. Remove corresponding TODO entry. Note: as shown by this patch, some needed fixes remain. Some options take yes/no, some 0/1, etc. --- TODO | 2 - bin/xen-create-image | 254 +++++++++++++++++++++++++++++-------------- 2 files changed, 171 insertions(+), 85 deletions(-) diff --git a/TODO b/TODO index 704ee40..10e7d0a 100644 --- a/TODO +++ b/TODO @@ -18,8 +18,6 @@ Minor bugs to fix and features to add before a 4.2 release stored in the configuration hash. The only issue is that trailing whitespace is missing from the "make_fs_foo" option. -* xen-create-image should check all integer options on non-digits. - * Test suite should pass Currently failing: diff --git a/bin/xen-create-image b/bin/xen-create-image index bc78667..1784605 100755 --- a/bin/xen-create-image +++ b/bin/xen-create-image @@ -1318,6 +1318,8 @@ sub setupDefaultOptions $CONFIG{ 'kernel' } = ''; $CONFIG{ 'modules' } = ''; $CONFIG{ 'initrd' } = ''; + $CONFIG{ 'serial_device' } = 'hvc0'; + $CONFIG{ 'disk_device' } = 'xvda'; # # Sizing options. @@ -1498,6 +1500,134 @@ sub readConfigurationFile +=begin doc + + Validate options and do what is necessary with them. + +=end doc + +=cut + +sub checkOption +{ + my ($option, $value) = @_; + + # Define argument types + my %types = ( + integerWithSuffix => { + check => qr/^[0-9.]+[GgMmKk]b?$/, + message => "takes a suffixed integer.\n", + }, + distribution => { + check => sub { -d "/usr/lib/xen-tools/$_[0].d" }, + message => "takes a distribution name " . + "(see /usr/lib/xen-tools for valid values).\n", + }, + imageType => { + check => qr/^sparse|full$/, + message => "must be 'sparse' or 'full'.\n", + }, + existingFile => { + check => sub { -e $_[0] }, + message => "must be an existing file.\n", + }, + existingDir => { + check => sub { -d $_[0] }, + message => "must be an existing directory.\n", + }, + serialDev => { + check => qr/^(?:\/dev\/)?(?:tty|hvc)[0-9]+$/, + message => "must be a serial device (tty*, hvc*).\n", + }, + diskDev => { + check => qr/^(?:\/dev\/)?(?:tty|hvc)[0-9]+$/, + message => "must be a disk device (tty*, hvc*).\n", + }, + ipv4 => { + check => qr/^(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)$/, + message => "must be valid IPv4.\n", + }, + hostname => { + check => qr/^[a-z0-9][a-z0-9.-]{254}$/i, + message => "must be a valid hostname.\n", + }, + supportedFs => { + check => qr/^(ext[234]|xfs|reiserfs)$/, + message => "must be a supported filesystem (ext2, ext3, ext4, xfs or reiserfs).\n", + }, + yesNo => { + check => qr/^yes|no$/i, + message => "must be 'yes' or 'no'.\n", + }, + zeroOne => { + check => qr/^0|1$/i, + message => "must be '0' or '1'.\n", + }, + configFile => { + check => sub { -e $_[0] or -e "/etc/xen-tools/" . $_[0] }, + message => "must be an existing file.\n", + }, + filename => { + check => qr/^[a-z0-9_.-]*$/, + message => "must be a valid filename.\n", + }, + ); + + # Define what argument each option accepts. + # Arguments for options not listed here will always be accepted. + my %optionsTypes = ( + size => 'integerWithSuffix', + dist => 'distribution', + swap => 'integerWithSuffix', + image => 'imageType', + memory => 'integerWithSuffix', + kernel => 'existingFile', + initrd => 'existingFile', + modules => 'existingDir', + serial_device => 'serialDev', + disk_device => 'diskDev', + gateway => 'ipv4', + netmask => 'ipv4', # This is dubious. + broadcast => 'ipv4', + hostname => 'hostname', + nameserver => 'ipv4', + pointopoint => 'ipv4', + fs => 'supportedFs', + cache => 'yesNo', + cachedir => 'existingDir', + config => 'configFile', + install => 'zeroOne', + hooks => 'zeroOne', + roledir => 'existingDir', + template => 'configFile', + output => 'existingDir', + extension => 'filename', + ); + + # If given option does not exists in optionsTypes, + # we just copy it to %CONFIG. + unless ( exists $optionsTypes{ $option } ) { + $CONFIG{ $option } = $value; + } else { # we validate it before copying + my $type = $optionsTypes{ $option }; + + # First, check if type exists + die unless exists $types{ $type }; + my $check = $types{ $type }{ 'check' }; + + if ( + (ref $check eq 'Regexp' and $value =~ $check) or + (ref $check eq 'CODE' and &$check( $value ) ) + ) { + # Option did validate, copy it + $CONFIG{ $option } = $value; + } else { + # Option did _not_ validate + die "ERROR: '$option' argument " . $types{ $type }{ 'message' }; + } + } +} + =begin doc Parse the command line arguments this script was given. @@ -1531,28 +1661,28 @@ sub parseCommandLineArguments !GetOptions( # Mandatory - "dist=s", \$CONFIG{ 'dist' }, + "dist=s", \&checkOption, # Size options. - "size=s", \$CONFIG{ 'size' }, - "swap=s", \$CONFIG{ 'swap' }, - "noswap", \$CONFIG{ 'noswap' }, - "image=s", \$CONFIG{ 'image' }, - "memory=s", \$CONFIG{ 'memory' }, - "vcpus=s", \$CONFIG{ 'vcpus' }, + "size=s", \&checkOption, + "swap=s", \&checkOption, + "noswap", \&checkOption, + "image=s", \&checkOption, + "memory=s", \&checkOption, + "vcpus=i", \&checkOption, # Locations "dir=s", \$install{ 'dir' }, "evms=s", \$install{ 'evms' }, - "kernel=s", \$CONFIG{ 'kernel' }, - "initrd=s", \$CONFIG{ 'initrd' }, - "mirror=s", \$CONFIG{ 'mirror' }, - "modules=s", \$CONFIG{ 'modules' }, + "kernel=s", \&checkOption, + "initrd=s", \&checkOption, + "mirror=s", \&checkOption, + "modules=s", \&checkOption, "lvm=s", \$install{ 'lvm' }, "image-dev=s", \$install{ 'image-dev' }, "swap-dev=s", \$install{ 'swap-dev' }, - "serial_device=s", \$CONFIG{ 'serial_device' }, - "disk_device=s", \$CONFIG{ 'disk_device' }, + "serial_device=s", \&checkOption, + "disk_device=s", \&checkOption, # Hosts options "nohosts", \$CONFIG{ 'nohosts' }, @@ -1563,16 +1693,16 @@ sub parseCommandLineArguments # Networking options "dhcp", \$CONFIG{ 'dhcp' }, - "bridge=s", \$CONFIG{ 'bridge' }, - "gateway=s", \$CONFIG{ 'gateway' }, - "hostname=s", \$CONFIG{ 'hostname' }, + "bridge=s", \&checkOption, + "gateway=s", \&checkOption, + "hostname=s", \&checkOption, "ip=s@", \$CONFIG{ 'ip' }, "mac=s", \$CONFIG{ 'mac' }, - "netmask=s", \$CONFIG{ 'netmask' }, - "broadcast=s", \$CONFIG{ 'broadcast' }, - "nameserver=s", \$CONFIG{ 'nameserver' }, - "vifname=s", \$CONFIG{ 'vifname' }, - "p2p=s", \$CONFIG{ 'p2p' }, + "netmask=s", \&checkOption, + "broadcast=s", \&checkOption, + "nameserver=s", \&checkOption, + "vifname=s", \&checkOption, + "p2p=s", \&checkOption, # Exclusive # @@ -1584,32 +1714,32 @@ sub parseCommandLineArguments # Misc. options "accounts", \$CONFIG{ 'accounts' }, - "admins=s", \$CONFIG{ 'admins' }, - "arch=s", \$CONFIG{ 'arch' }, - "fs=s", \$CONFIG{ 'fs' }, + "admins=s", \&checkOption, + "arch=s", \&checkOption, + "fs=s", \&checkOption, "boot", \$CONFIG{ 'boot' }, - "cache=s", \$CONFIG{ 'cache' }, - "cachedir=s", \$CONFIG{ 'cachedir' }, - "config=s", \$CONFIG{ 'config' }, + "cache=s", \&checkOption, + "cachedir=s", \&checkOption, + "config=s", \&checkOption, "ide", \$CONFIG{ 'ide' }, "scsi", \$CONFIG{ 'scsi' }, - "install=i", \$CONFIG{ 'install' }, - "hooks=i", \$CONFIG{ 'hooks' }, + "install=i", \&checkOption, + "hooks=i", \&checkOption, "pygrub", \$CONFIG{ 'pygrub' }, "passwd", \$CONFIG{ 'passwd' }, - "genpass=i", \$CONFIG{ 'genpass' }, - "genpass-len=i", \$CONFIG{ 'genpass_len' }, - "genpass_len=i", \$CONFIG{ 'genpass_len' }, - "password=s", \$CONFIG{ 'password' }, - "partitions=s", \$CONFIG{ 'partitions' }, - "role=s", \$CONFIG{ 'role' }, - "role-args=s", \$CONFIG{ 'role-args' }, - "roledir=s", \$CONFIG{ 'roledir' }, + "genpass=i", \&checkOption, + "genpass-len=i", \&checkOption, + "genpass_len=i", \&checkOption, + "password=s", \&checkOption, + "partitions=s", \&checkOption, + "role=s", \&checkOption, + "role-args=s", \&checkOption, + "roledir=s", \&checkOption, "force", \$CONFIG{ 'force' }, "keep", \$CONFIG{ 'keep' }, - "template=s", \$CONFIG{ 'template' }, - "output=s", \$CONFIG{ 'output' }, - "extension=s", \$CONFIG{ 'extension' }, + "template=s", \&checkOption, + "output=s", \&checkOption, + "extension=s", \&checkOption, # Help options "debug", \$CONFIG{ 'debug' }, @@ -1619,6 +1749,7 @@ sub parseCommandLineArguments "version", \$VERSION ) ) { + $FAIL = 2; exit; } @@ -1786,49 +1917,6 @@ sub checkArguments $CONFIG{ 'dist' } = 'stentz'; } - # - # - # Test that the distribution name we've been given - # to configure has a collection of hook scripts. - # - # If there are no scripts then we clearly cannot - # customise it! - # - my $dir = "/usr/lib/xen-tools/" . $CONFIG{ 'dist' } . ".d"; - - if ( !-d $dir ) - { - my $err = <