On 2/25/20 12:32 PM, Fabian Grünbichler wrote:
this is missing some stuff (line numbers refer to after all patches have
been applied):

PVE/API2/Qemu.pm:1061:        if (PVE::QemuServer::is_valid_drivename($opt)) {
PVE/API2/Qemu.pm:1063:            my $drive = PVE::QemuServer::parse_drive($opt, 
$param->{$opt});
PVE/API2/Qemu.pm:1067:            $param->{$opt} = 
PVE::QemuServer::print_drive($drive);
PVE/API2/Qemu.pm:1147:                    my $drive = 
PVE::QemuServer::parse_drive($opt, $val);
PVE/API2/Qemu.pm:1160:                } elsif 
(PVE::QemuServer::is_valid_drivename($opt)) {
PVE/API2/Qemu.pm:1163:                    
PVE::QemuServer::vmconfig_register_unused_drive($storecfg, $vmid, $conf, 
PVE::QemuServer::parse_drive($opt, $val))
PVE/API2/Qemu.pm:1196:                if 
(PVE::QemuServer::is_valid_drivename($opt)) {
PVE/API2/Qemu.pm:1197:                    my $drive = 
PVE::QemuServer::parse_drive($opt, $param->{$opt});
PVE/API2/Qemu.pm:1204:                    
PVE::QemuServer::vmconfig_register_unused_drive($storecfg, $vmid, $conf, 
PVE::QemuServer::parse_drive($opt, $conf->{pending}->{$opt}))
PVE/API2/Qemu.pm:150:     $res->{$ds} = PVE::QemuServer::print_drive($disk);
PVE/API2/Qemu.pm:172:     $res->{$ds} = PVE::QemuServer::print_drive($disk);
PVE/API2/Qemu.pm:191:     $res->{$ds} = PVE::QemuServer::print_drive($disk);
PVE/API2/Qemu.pm:199:         my $olddrive = PVE::QemuServer::parse_drive($ds, 
$conf->{$ds});
PVE/API2/Qemu.pm:214:     $res->{$ds} = PVE::QemuServer::print_drive($disk);
PVE/API2/Qemu.pm:218:    eval { PVE::QemuServer::foreach_drive($settings, 
$code); };
PVE/API2/Qemu.pm:2832:                } elsif 
(PVE::QemuServer::is_valid_drivename($opt)) {
PVE/API2/Qemu.pm:2833:                    my $drive = 
PVE::QemuServer::parse_drive($opt, $value);
PVE/API2/Qemu.pm:2913:                        $newconf->{$opt} = 
PVE::QemuServer::print_drive($newdrive);
PVE/API2/Qemu.pm:2995:                enum => [ 
PVE::QemuServer::valid_drive_names() ],
PVE/API2/Qemu.pm:3056:            my $drive = PVE::QemuServer::parse_drive($disk, 
$conf->{$disk});
PVE/API2/Qemu.pm:3100:                    $conf->{$disk} = 
PVE::QemuServer::print_drive($newdrive);
PVE/API2/Qemu.pm:317: next if PVE::QemuServer::is_valid_drivename($opt);
PVE/API2/Qemu.pm:3490:                enum => 
[PVE::QemuServer::valid_drive_names()],
PVE/API2/Qemu.pm:3539:            my $drive = PVE::QemuServer::parse_drive($disk, 
$conf->{$disk});
PVE/API2/Qemu.pm:3588:            $conf->{$disk} = 
PVE::QemuServer::print_drive($drive);
PVE/API2/Qemu.pm:3990:                enum => 
[PVE::QemuServer::valid_drive_names()],
PVE/API2/Qemu.pm:526:         if (PVE::QemuServer::is_valid_drivename($opt)) {
PVE/API2/Qemu.pm:527:             my $drive = PVE::QemuServer::parse_drive($opt, 
$param->{$opt});
PVE/API2/Qemu.pm:531:             $param->{$opt} = 
PVE::QemuServer::print_drive($drive);
PVE/API2/Qemu.pm:65:   PVE::QemuServer::foreach_drive($settings, sub {
PVE/API2/Qemu.pm:98:   PVE::QemuServer::foreach_drive($conf, sub {
PVE/QemuConfig.pm:291:        my $drive = 
PVE::QemuServer::parse_drive($remove_drive, $snap->{$remove_drive});
PVE/QemuConfig.pm:424:    PVE::QemuServer::foreach_drive($conf, $func, @param);
PVE/QemuConfig.pm:70:    PVE::QemuServer::foreach_drive($conf, sub {
PVE/QemuMigrate.pm:452:       PVE::QemuServer::foreach_drive($conf, sub {
PVE/QemuMigrate.pm:456:               $conf->{$key} = 
PVE::QemuServer::print_drive($updated);
PVE/QemuMigrate.pm:511:       my $drive = PVE::QemuServer::parse_drive($target_drive, 
$self->{target_drive}->{$target_drive}->{drivestr});
PVE/QemuMigrate.pm:640:           my $current_drive = 
PVE::QemuServer::parse_drive($targetdrive, $conf->{$targetdrive});
PVE/QemuMigrate.pm:641:           my $new_drive = 
PVE::QemuServer::parse_drive($targetdrive, $drivestr);
PVE/QemuMigrate.pm:714:           my $source_drive = 
PVE::QemuServer::parse_drive($drive, $conf->{$drive});
PVE/QemuMigrate.pm:715:           my $target_drive = 
PVE::QemuServer::parse_drive($drive, $target->{drivestr});
PVE/QemuServer/Cloudinit.pm:467:    PVE::QemuServer::foreach_drive($conf, sub {
PVE/QemuServer/ImportDisk.pm:19:    if ($drive_name && 
!(PVE::QemuServer::is_valid_drivename($drive_name))) {
PVE/QemuServer.pm:3974:       if (PVE::QemuServer::is_valid_drivename($opt)) {
PVE/QemuServer.pm:3975:           my $drive = PVE::QemuServer::parse_drive($opt, 
$conf->{$opt});
PVE/QemuServer.pm:6755:    my @disks = PVE::QemuServer::valid_drive_names();
PVE/QemuServer.pm:6759:       my $disk = PVE::QemuServer::parse_drive($ds, 
$conf->{$ds});
PVE/VZDump/QemuServer.pm:72:    PVE::QemuServer::foreach_drive($conf, sub {

which should have been noticed while testing..


I'm a bit confused what you mean by missing? The commit message mentions why changing the call sites isn't neccessary. If you want to, I can do that for the next version, but I thought exports would be fine.

$unuseddesc could also move to Drive.pm, especially since we have talked
for quite a while that we might want to include more information than
just the volume ID in the future.

other possible candidates (also maybe for a separate follow-up, since
these should mostly be qemu-server internal without need of
intra-package dependencies):
- pve-qm-bootdisk and its verification sub
- resolve_first_disk
- unused disk cleanup in write_vm_config
- drive_is_cloudinit
- drive_is_cdrom
- disksize
- foreach_volid (either moved as-is to Drive.pm, or as variant of
   foreach_volume in AbstractConfig.pm?)
- is_volume_in_use
- update_disksize
- update_disk_config (would need some work to avoid usage of
   PVE::QemuConfig)



Ok, I'll take a look at those and see how easily they can be moved. I'll send the refactoring as its own series next time, since it isn't strictly related to the migration part of the series.

PVE::QemuServer::Drive::print_drive and
PVE::QemuServer::print_drive_full do totally different stuff, it might
be a good idea to rename either to avoid confusion (not new in this
series, but we might as well when we are at it ;)). print_drive_full
actually generates the 'drive' string for the Qemu commandline, so maybe
something like drive_to_qemu_string or print_drive_qemu (as always,
naming is not my strength ;))?

Ok.

On February 24, 2020 1:43 pm, Fabian Ebner wrote:
Apart from moving and adapting two places where $MAX_SATA_DISKS is used,
the initialization for the drive keys in $confdesc is changed
to be a single for-loop iterating over the keys of $drivedesc_hash.

To avoid the need to change all the call sites, the functions are
exported from the submodule and imported into QemuServer.pm.

Signed-off-by: Fabian Ebner <f.eb...@proxmox.com>
---
  PVE/QemuServer.pm       | 459 +--------------------------------------
  PVE/QemuServer/Drive.pm | 468 ++++++++++++++++++++++++++++++++++++++++
  PVE/QemuServer/Makefile |   1 +
  3 files changed, 474 insertions(+), 454 deletions(-)
  create mode 100644 PVE/QemuServer/Drive.pm

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 1580514..cf6e096 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -44,6 +44,7 @@ use PVE::QemuConfig;
  use PVE::QemuServer::Helpers qw(min_version config_aware_timeout);
  use PVE::QemuServer::Cloudinit;
  use PVE::QemuServer::CPUConfig qw(print_cpu_device get_cpu_options);
+use PVE::QemuServer::Drive qw(valid_drive_names is_valid_drivename parse_drive 
print_drive foreach_drive);
  use PVE::QemuServer::Machine;
  use PVE::QemuServer::Memory;
  use PVE::QemuServer::Monitor qw(mon_cmd);
@@ -84,13 +85,6 @@ PVE::JSONSchema::register_standard_option('pve-qm-stateuri', 
{
      optional => 1,
  });
-PVE::JSONSchema::register_standard_option('pve-qm-image-format', {
-    type => 'string',
-    enum => [qw(raw cow qcow qed qcow2 vmdk cloop)],
-    description => "The drive's backing file's data format.",
-    optional => 1,
-});
-
  PVE::JSONSchema::register_standard_option('pve-qemu-machine', {
        description => "Specifies the Qemu machine type.",
        type => 'string',
@@ -696,10 +690,6 @@ while (my ($k, $v) = each %$confdesc) {
      PVE::JSONSchema::register_standard_option("pve-qm-$k", $v);
  }
-my $MAX_IDE_DISKS = 4;
-my $MAX_SCSI_DISKS = 31;
-my $MAX_VIRTIO_DISKS = 16;
-my $MAX_SATA_DISKS = 6;
  my $MAX_USB_DEVICES = 5;
  my $MAX_NETS = 32;
  my $MAX_UNUSED_DISKS = 256;
@@ -905,310 +895,6 @@ sub verify_volume_id_or_qm_path {
      return $volid;
  }
-my $drivedesc_hash;
-
-my %drivedesc_base = (
-    volume => { alias => 'file' },
-    file => {
-       type => 'string',
-       format => 'pve-volume-id-or-qm-path',
-       default_key => 1,
-       format_description => 'volume',
-       description => "The drive's backing volume.",
-    },
-    media => {
-       type => 'string',
-       enum => [qw(cdrom disk)],
-       description => "The drive's media type.",
-       default => 'disk',
-       optional => 1
-    },
-    cyls => {
-       type => 'integer',
-       description => "Force the drive's physical geometry to have a specific 
cylinder count.",
-       optional => 1
-    },
-    heads => {
-       type => 'integer',
-       description => "Force the drive's physical geometry to have a specific head 
count.",
-       optional => 1
-    },
-    secs => {
-       type => 'integer',
-       description => "Force the drive's physical geometry to have a specific 
sector count.",
-       optional => 1
-    },
-    trans => {
-       type => 'string',
-       enum => [qw(none lba auto)],
-       description => "Force disk geometry bios translation mode.",
-       optional => 1,
-    },
-    snapshot => {
-       type => 'boolean',
-       description => "Controls qemu's snapshot mode feature."
-           . " If activated, changes made to the disk are temporary and will"
-           . " be discarded when the VM is shutdown.",
-       optional => 1,
-    },
-    cache => {
-       type => 'string',
-       enum => [qw(none writethrough writeback unsafe directsync)],
-       description => "The drive's cache mode",
-       optional => 1,
-    },
-    format => get_standard_option('pve-qm-image-format'),
-    size => {
-       type => 'string',
-       format => 'disk-size',
-       format_description => 'DiskSize',
-       description => "Disk size. This is purely informational and has no 
effect.",
-       optional => 1,
-    },
-    backup => {
-       type => 'boolean',
-       description => "Whether the drive should be included when making 
backups.",
-       optional => 1,
-    },
-    replicate => {
-       type => 'boolean',
-       description => 'Whether the drive should considered for replication 
jobs.',
-       optional => 1,
-       default => 1,
-    },
-    rerror => {
-       type => 'string',
-       enum => [qw(ignore report stop)],
-       description => 'Read error action.',
-       optional => 1,
-    },
-    werror => {
-       type => 'string',
-       enum => [qw(enospc ignore report stop)],
-       description => 'Write error action.',
-       optional => 1,
-    },
-    aio => {
-       type => 'string',
-       enum => [qw(native threads)],
-       description => 'AIO type to use.',
-       optional => 1,
-    },
-    discard => {
-       type => 'string',
-       enum => [qw(ignore on)],
-       description => 'Controls whether to pass discard/trim requests to the 
underlying storage.',
-       optional => 1,
-    },
-    detect_zeroes => {
-       type => 'boolean',
-       description => 'Controls whether to detect and try to optimize writes 
of zeroes.',
-       optional => 1,
-    },
-    serial => {
-       type => 'string',
-       format => 'urlencoded',
-       format_description => 'serial',
-       maxLength => 20*3, # *3 since it's %xx url enoded
-       description => "The drive's reported serial number, url-encoded, up to 20 
bytes long.",
-       optional => 1,
-    },
-    shared => {
-       type => 'boolean',
-       description => 'Mark this locally-managed volume as available on all 
nodes',
-       verbose_description => "Mark this locally-managed volume as available on all 
nodes.\n\nWARNING: This option does not share the volume automatically, it assumes it is 
shared already!",
-       optional => 1,
-       default => 0,
-    }
-);
-
-my %iothread_fmt = ( iothread => {
-       type => 'boolean',
-       description => "Whether to use iothreads for this drive",
-       optional => 1,
-});
-
-my %model_fmt = (
-    model => {
-       type => 'string',
-       format => 'urlencoded',
-       format_description => 'model',
-       maxLength => 40*3, # *3 since it's %xx url enoded
-       description => "The drive's reported model name, url-encoded, up to 40 bytes 
long.",
-       optional => 1,
-    },
-);
-
-my %queues_fmt = (
-    queues => {
-       type => 'integer',
-       description => "Number of queues.",
-       minimum => 2,
-       optional => 1
-    }
-);
-
-my %scsiblock_fmt = (
-    scsiblock => {
-       type => 'boolean',
-       description => "whether to use scsi-block for full passthrough of host block 
device\n\nWARNING: can lead to I/O errors in combination with low memory or high memory 
fragmentation on host",
-       optional => 1,
-       default => 0,
-    },
-);
-
-my %ssd_fmt = (
-    ssd => {
-       type => 'boolean',
-       description => "Whether to expose this drive as an SSD, rather than a 
rotational hard disk.",
-       optional => 1,
-    },
-);
-
-my %wwn_fmt = (
-    wwn => {
-       type => 'string',
-       pattern => qr/^(0x)[0-9a-fA-F]{16}/,
-       format_description => 'wwn',
-       description => "The drive's worldwide name, encoded as 16 bytes hex string, 
prefixed by '0x'.",
-       optional => 1,
-    },
-);
-
-my $add_throttle_desc = sub {
-    my ($key, $type, $what, $unit, $longunit, $minimum) = @_;
-    my $d = {
-       type => $type,
-       format_description => $unit,
-       description => "Maximum $what in $longunit.",
-       optional => 1,
-    };
-    $d->{minimum} = $minimum if defined($minimum);
-    $drivedesc_base{$key} = $d;
-};
-# throughput: (leaky bucket)
-$add_throttle_desc->('bps',     'integer', 'r/w speed',   'bps',  'bytes per 
second');
-$add_throttle_desc->('bps_rd',  'integer', 'read speed',  'bps',  'bytes per 
second');
-$add_throttle_desc->('bps_wr',  'integer', 'write speed', 'bps',  'bytes per 
second');
-$add_throttle_desc->('mbps',    'number',  'r/w speed',   'mbps', 'megabytes 
per second');
-$add_throttle_desc->('mbps_rd', 'number',  'read speed',  'mbps', 'megabytes 
per second');
-$add_throttle_desc->('mbps_wr', 'number',  'write speed', 'mbps', 'megabytes 
per second');
-$add_throttle_desc->('iops',    'integer', 'r/w I/O',     'iops', 'operations 
per second');
-$add_throttle_desc->('iops_rd', 'integer', 'read I/O',    'iops', 'operations 
per second');
-$add_throttle_desc->('iops_wr', 'integer', 'write I/O',   'iops', 'operations 
per second');
-
-# pools: (pool of IO before throttling starts taking effect)
-$add_throttle_desc->('mbps_max',    'number',  'unthrottled r/w pool',       
'mbps', 'megabytes per second');
-$add_throttle_desc->('mbps_rd_max', 'number',  'unthrottled read pool',      
'mbps', 'megabytes per second');
-$add_throttle_desc->('mbps_wr_max', 'number',  'unthrottled write pool',     
'mbps', 'megabytes per second');
-$add_throttle_desc->('iops_max',    'integer', 'unthrottled r/w I/O pool',   
'iops', 'operations per second');
-$add_throttle_desc->('iops_rd_max', 'integer', 'unthrottled read I/O pool',  
'iops', 'operations per second');
-$add_throttle_desc->('iops_wr_max', 'integer', 'unthrottled write I/O pool', 
'iops', 'operations per second');
-
-# burst lengths
-$add_throttle_desc->('bps_max_length',     'integer', 'length of I/O bursts',  
     'seconds', 'seconds', 1);
-$add_throttle_desc->('bps_rd_max_length',  'integer', 'length of read I/O 
bursts',  'seconds', 'seconds', 1);
-$add_throttle_desc->('bps_wr_max_length',  'integer', 'length of write I/O 
bursts', 'seconds', 'seconds', 1);
-$add_throttle_desc->('iops_max_length',    'integer', 'length of I/O bursts',  
     'seconds', 'seconds', 1);
-$add_throttle_desc->('iops_rd_max_length', 'integer', 'length of read I/O 
bursts',  'seconds', 'seconds', 1);
-$add_throttle_desc->('iops_wr_max_length', 'integer', 'length of write I/O 
bursts', 'seconds', 'seconds', 1);
-
-# legacy support
-$drivedesc_base{'bps_rd_length'} = { alias => 'bps_rd_max_length' };
-$drivedesc_base{'bps_wr_length'} = { alias => 'bps_wr_max_length' };
-$drivedesc_base{'iops_rd_length'} = { alias => 'iops_rd_max_length' };
-$drivedesc_base{'iops_wr_length'} = { alias => 'iops_wr_max_length' };
-
-my $ide_fmt = {
-    %drivedesc_base,
-    %model_fmt,
-    %ssd_fmt,
-    %wwn_fmt,
-};
-PVE::JSONSchema::register_format("pve-qm-ide", $ide_fmt);
-
-my $idedesc = {
-    optional => 1,
-    type => 'string', format => $ide_fmt,
-    description => "Use volume as IDE hard disk or CD-ROM (n is 0 to " .($MAX_IDE_DISKS 
-1) . ").",
-};
-PVE::JSONSchema::register_standard_option("pve-qm-ide", $idedesc);
-
-my $scsi_fmt = {
-    %drivedesc_base,
-    %iothread_fmt,
-    %queues_fmt,
-    %scsiblock_fmt,
-    %ssd_fmt,
-    %wwn_fmt,
-};
-my $scsidesc = {
-    optional => 1,
-    type => 'string', format => $scsi_fmt,
-    description => "Use volume as SCSI hard disk or CD-ROM (n is 0 to " . 
($MAX_SCSI_DISKS - 1) . ").",
-};
-PVE::JSONSchema::register_standard_option("pve-qm-scsi", $scsidesc);
-
-my $sata_fmt = {
-    %drivedesc_base,
-    %ssd_fmt,
-    %wwn_fmt,
-};
-my $satadesc = {
-    optional => 1,
-    type => 'string', format => $sata_fmt,
-    description => "Use volume as SATA hard disk or CD-ROM (n is 0 to " . 
($MAX_SATA_DISKS - 1). ").",
-};
-PVE::JSONSchema::register_standard_option("pve-qm-sata", $satadesc);
-
-my $virtio_fmt = {
-    %drivedesc_base,
-    %iothread_fmt,
-};
-my $virtiodesc = {
-    optional => 1,
-    type => 'string', format => $virtio_fmt,
-    description => "Use volume as VIRTIO hard disk (n is 0 to " . ($MAX_VIRTIO_DISKS - 
1) . ").",
-};
-PVE::JSONSchema::register_standard_option("pve-qm-virtio", $virtiodesc);
-
-my $alldrive_fmt = {
-    %drivedesc_base,
-    %iothread_fmt,
-    %model_fmt,
-    %queues_fmt,
-    %scsiblock_fmt,
-    %ssd_fmt,
-    %wwn_fmt,
-};
-
-my $efidisk_fmt = {
-    volume => { alias => 'file' },
-    file => {
-       type => 'string',
-       format => 'pve-volume-id-or-qm-path',
-       default_key => 1,
-       format_description => 'volume',
-       description => "The drive's backing volume.",
-    },
-    format => get_standard_option('pve-qm-image-format'),
-    size => {
-       type => 'string',
-       format => 'disk-size',
-       format_description => 'DiskSize',
-       description => "Disk size. This is purely informational and has no 
effect.",
-       optional => 1,
-    },
-};
-
-my $efidisk_desc = {
-    optional => 1,
-    type => 'string', format => $efidisk_fmt,
-    description => "Configure a Disk for storing EFI vars",
-};
-
-PVE::JSONSchema::register_standard_option("pve-qm-efidisk", $efidisk_desc);
-
  my $usb_fmt = {
      host => {
        default_key => 1,
@@ -1355,29 +1041,10 @@ for (my $i = 0; $i < $MAX_HOSTPCI_DEVICES; $i++)  {
      $confdesc->{"hostpci$i"} = $hostpcidesc;
  }
-for (my $i = 0; $i < $MAX_IDE_DISKS; $i++) {
-    $drivedesc_hash->{"ide$i"} = $idedesc;
-    $confdesc->{"ide$i"} = $idedesc;
-}
-
-for (my $i = 0; $i < $MAX_SATA_DISKS; $i++)  {
-    $drivedesc_hash->{"sata$i"} = $satadesc;
-    $confdesc->{"sata$i"} = $satadesc;
-}
-
-for (my $i = 0; $i < $MAX_SCSI_DISKS; $i++)  {
-    $drivedesc_hash->{"scsi$i"} = $scsidesc;
-    $confdesc->{"scsi$i"} = $scsidesc ;
+for my $key (keys %{$PVE::QemuServer::Drive::drivedesc_hash}) {
+    $confdesc->{$key} = $PVE::QemuServer::Drive::drivedesc_hash->{$key};
  }
-for (my $i = 0; $i < $MAX_VIRTIO_DISKS; $i++) {
-    $drivedesc_hash->{"virtio$i"} = $virtiodesc;
-    $confdesc->{"virtio$i"} = $virtiodesc;
-}
-
-$drivedesc_hash->{efidisk0} = $efidisk_desc;
-$confdesc->{efidisk0} = $efidisk_desc;
-
  for (my $i = 0; $i < $MAX_USB_DEVICES; $i++)  {
      $confdesc->{"usb$i"} = $usbdesc;
  }
@@ -1440,21 +1107,6 @@ sub kernel_has_vhost_net {
      return -c '/dev/vhost-net';
  }
-sub valid_drive_names {
-    # order is important - used to autoselect boot disk
-    return ((map { "ide$_" } (0 .. ($MAX_IDE_DISKS - 1))),
-            (map { "scsi$_" } (0 .. ($MAX_SCSI_DISKS - 1))),
-            (map { "virtio$_" } (0 .. ($MAX_VIRTIO_DISKS - 1))),
-            (map { "sata$_" } (0 .. ($MAX_SATA_DISKS - 1))),
-            'efidisk0');
-}
-
-sub is_valid_drivename {
-    my $dev = shift;
-
-    return defined($drivedesc_hash->{$dev});
-}
-
  sub option_exists {
      my $key = shift;
      return defined($confdesc->{$key});
@@ -1571,94 +1223,6 @@ sub pve_verify_hotplug_features {
      die "unable to parse hotplug option\n";
  }
-# ideX = [volume=]volume-id[,media=d][,cyls=c,heads=h,secs=s[,trans=t]]
-#        [,snapshot=on|off][,cache=on|off][,format=f][,backup=yes|no]
-#        [,rerror=ignore|report|stop][,werror=enospc|ignore|report|stop]
-#        [,aio=native|threads][,discard=ignore|on][,detect_zeroes=on|off]
-#        [,iothread=on][,serial=serial][,model=model]
-
-sub parse_drive {
-    my ($key, $data) = @_;
-
-    my ($interface, $index);
-
-    if ($key =~ m/^([^\d]+)(\d+)$/) {
-       $interface = $1;
-       $index = $2;
-    } else {
-       return undef;
-    }
-
-    my $desc = $key =~ /^unused\d+$/ ? $alldrive_fmt
-                                     : $drivedesc_hash->{$key}->{format};
-    if (!$desc) {
-       warn "invalid drive key: $key\n";
-       return undef;
-    }
-    my $res = eval { PVE::JSONSchema::parse_property_string($desc, $data) };
-    return undef if !$res;
-    $res->{interface} = $interface;
-    $res->{index} = $index;
-
-    my $error = 0;
-    foreach my $opt (qw(bps bps_rd bps_wr)) {
-       if (my $bps = defined(delete $res->{$opt})) {
-           if (defined($res->{"m$opt"})) {
-               warn "both $opt and m$opt specified\n";
-               ++$error;
-               next;
-           }
-           $res->{"m$opt"} = sprintf("%.3f", $bps / (1024*1024.0));
-       }
-    }
-
-    # can't use the schema's 'requires' because of the mbps* => bps* "transforming 
aliases"
-    for my $requirement (
-       [mbps_max => 'mbps'],
-       [mbps_rd_max => 'mbps_rd'],
-       [mbps_wr_max => 'mbps_wr'],
-       [miops_max => 'miops'],
-       [miops_rd_max => 'miops_rd'],
-       [miops_wr_max => 'miops_wr'],
-       [bps_max_length => 'mbps_max'],
-       [bps_rd_max_length => 'mbps_rd_max'],
-       [bps_wr_max_length => 'mbps_wr_max'],
-       [iops_max_length => 'iops_max'],
-       [iops_rd_max_length => 'iops_rd_max'],
-       [iops_wr_max_length => 'iops_wr_max']) {
-       my ($option, $requires) = @$requirement;
-       if ($res->{$option} && !$res->{$requires}) {
-           warn "$option requires $requires\n";
-           ++$error;
-       }
-    }
-
-    return undef if $error;
-
-    return undef if $res->{mbps_rd} && $res->{mbps};
-    return undef if $res->{mbps_wr} && $res->{mbps};
-    return undef if $res->{iops_rd} && $res->{iops};
-    return undef if $res->{iops_wr} && $res->{iops};
-
-    if ($res->{media} && ($res->{media} eq 'cdrom')) {
-       return undef if $res->{snapshot} || $res->{trans} || $res->{format};
-       return undef if $res->{heads} || $res->{secs} || $res->{cyls};
-       return undef if $res->{interface} eq 'virtio';
-    }
-
-    if (my $size = $res->{size}) {
-       return undef if !defined($res->{size} = 
PVE::JSONSchema::parse_size($size));
-    }
-
-    return $res;
-}
-
-sub print_drive {
-    my ($drive) = @_;
-    my $skip = [ 'index', 'interface' ];
-    return PVE::JSONSchema::print_property_string($drive, $alldrive_fmt, 
$skip);
-}
-
  sub scsi_inquiry {
      my($fh, $noerr) = @_;
@@ -1796,7 +1360,7 @@ sub print_drivedevice_full {
        $device .= ",wwn=$drive->{wwn}" if $drive->{wwn};
} elsif ($drive->{interface} eq 'ide' || $drive->{interface} eq 'sata') {
-       my $maxdev = ($drive->{interface} eq 'sata') ? $MAX_SATA_DISKS : 2;
+       my $maxdev = ($drive->{interface} eq 'sata') ? 
$PVE::QemuServer::Drive::MAX_SATA_DISKS : 2;
        my $controller = int($drive->{index} / $maxdev);
        my $unit = $drive->{index} % $maxdev;
        my $devicetype = ($drive->{media} && $drive->{media} eq 'cdrom') ? "cd" : 
"hd";
@@ -3125,19 +2689,6 @@ sub vmstatus {
      return $res;
  }
-sub foreach_drive {
-    my ($conf, $func, @param) = @_;
-
-    foreach my $ds (valid_drive_names()) {
-       next if !defined($conf->{$ds});
-
-       my $drive = parse_drive($ds, $conf->{$ds});
-       next if !$drive;
-
-       &$func($ds, $drive, @param);
-    }
-}
-
  sub foreach_volid {
      my ($conf, $func, @param) = @_;
@@ -3950,7 +3501,7 @@ sub config_to_command {
          }
if ($drive->{interface} eq 'sata') {
-           my $controller = int($drive->{index} / $MAX_SATA_DISKS);
+           my $controller = int($drive->{index} / 
$PVE::QemuServer::Drive::MAX_SATA_DISKS);
             $pciaddr = print_pci_addr("ahci$controller", $bridges, $arch, 
$machine_type);
             push @$devices, '-device', 
"ahci,id=ahci$controller,multifunction=on$pciaddr" if 
!$ahcicontroller->{$controller};
             $ahcicontroller->{$controller}=1;
diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
new file mode 100644
index 0000000..35ac99d
--- /dev/null
+++ b/PVE/QemuServer/Drive.pm
@@ -0,0 +1,468 @@
+package PVE::QemuServer::Drive;
+
+use strict;
+use warnings;
+
+use PVE::JSONSchema qw(get_standard_option);
+
+use base qw(Exporter);
+
+our @EXPORT_OK = qw(
+valid_drive_names
+is_valid_drivename
+parse_drive
+print_drive
+foreach_drive
+);
+
+PVE::JSONSchema::register_standard_option('pve-qm-image-format', {
+    type => 'string',
+    enum => [qw(raw cow qcow qed qcow2 vmdk cloop)],
+    description => "The drive's backing file's data format.",
+    optional => 1,
+});
+
+our $MAX_IDE_DISKS = 4;
+our $MAX_SCSI_DISKS = 31;
+our $MAX_VIRTIO_DISKS = 16;
+our $MAX_SATA_DISKS = 6;
+
+our $drivedesc_hash;
+
+my %drivedesc_base = (
+    volume => { alias => 'file' },
+    file => {
+       type => 'string',
+       format => 'pve-volume-id-or-qm-path',
+       default_key => 1,
+       format_description => 'volume',
+       description => "The drive's backing volume.",
+    },
+    media => {
+       type => 'string',
+       enum => [qw(cdrom disk)],
+       description => "The drive's media type.",
+       default => 'disk',
+       optional => 1
+    },
+    cyls => {
+       type => 'integer',
+       description => "Force the drive's physical geometry to have a specific 
cylinder count.",
+       optional => 1
+    },
+    heads => {
+       type => 'integer',
+       description => "Force the drive's physical geometry to have a specific head 
count.",
+       optional => 1
+    },
+    secs => {
+       type => 'integer',
+       description => "Force the drive's physical geometry to have a specific 
sector count.",
+       optional => 1
+    },
+    trans => {
+       type => 'string',
+       enum => [qw(none lba auto)],
+       description => "Force disk geometry bios translation mode.",
+       optional => 1,
+    },
+    snapshot => {
+       type => 'boolean',
+       description => "Controls qemu's snapshot mode feature."
+           . " If activated, changes made to the disk are temporary and will"
+           . " be discarded when the VM is shutdown.",
+       optional => 1,
+    },
+    cache => {
+       type => 'string',
+       enum => [qw(none writethrough writeback unsafe directsync)],
+       description => "The drive's cache mode",
+       optional => 1,
+    },
+    format => get_standard_option('pve-qm-image-format'),
+    size => {
+       type => 'string',
+       format => 'disk-size',
+       format_description => 'DiskSize',
+       description => "Disk size. This is purely informational and has no 
effect.",
+       optional => 1,
+    },
+    backup => {
+       type => 'boolean',
+       description => "Whether the drive should be included when making 
backups.",
+       optional => 1,
+    },
+    replicate => {
+       type => 'boolean',
+       description => 'Whether the drive should considered for replication 
jobs.',
+       optional => 1,
+       default => 1,
+    },
+    rerror => {
+       type => 'string',
+       enum => [qw(ignore report stop)],
+       description => 'Read error action.',
+       optional => 1,
+    },
+    werror => {
+       type => 'string',
+       enum => [qw(enospc ignore report stop)],
+       description => 'Write error action.',
+       optional => 1,
+    },
+    aio => {
+       type => 'string',
+       enum => [qw(native threads)],
+       description => 'AIO type to use.',
+       optional => 1,
+    },
+    discard => {
+       type => 'string',
+       enum => [qw(ignore on)],
+       description => 'Controls whether to pass discard/trim requests to the 
underlying storage.',
+       optional => 1,
+    },
+    detect_zeroes => {
+       type => 'boolean',
+       description => 'Controls whether to detect and try to optimize writes 
of zeroes.',
+       optional => 1,
+    },
+    serial => {
+       type => 'string',
+       format => 'urlencoded',
+       format_description => 'serial',
+       maxLength => 20*3, # *3 since it's %xx url enoded
+       description => "The drive's reported serial number, url-encoded, up to 20 
bytes long.",
+       optional => 1,
+    },
+    shared => {
+       type => 'boolean',
+       description => 'Mark this locally-managed volume as available on all 
nodes',
+       verbose_description => "Mark this locally-managed volume as available on all 
nodes.\n\nWARNING: This option does not share the volume automatically, it assumes it is 
shared already!",
+       optional => 1,
+       default => 0,
+    }
+);
+
+my %iothread_fmt = ( iothread => {
+       type => 'boolean',
+       description => "Whether to use iothreads for this drive",
+       optional => 1,
+});
+
+my %model_fmt = (
+    model => {
+       type => 'string',
+       format => 'urlencoded',
+       format_description => 'model',
+       maxLength => 40*3, # *3 since it's %xx url enoded
+       description => "The drive's reported model name, url-encoded, up to 40 bytes 
long.",
+       optional => 1,
+    },
+);
+
+my %queues_fmt = (
+    queues => {
+       type => 'integer',
+       description => "Number of queues.",
+       minimum => 2,
+       optional => 1
+    }
+);
+
+my %scsiblock_fmt = (
+    scsiblock => {
+       type => 'boolean',
+       description => "whether to use scsi-block for full passthrough of host block 
device\n\nWARNING: can lead to I/O errors in combination with low memory or high memory 
fragmentation on host",
+       optional => 1,
+       default => 0,
+    },
+);
+
+my %ssd_fmt = (
+    ssd => {
+       type => 'boolean',
+       description => "Whether to expose this drive as an SSD, rather than a 
rotational hard disk.",
+       optional => 1,
+    },
+);
+
+my %wwn_fmt = (
+    wwn => {
+       type => 'string',
+       pattern => qr/^(0x)[0-9a-fA-F]{16}/,
+       format_description => 'wwn',
+       description => "The drive's worldwide name, encoded as 16 bytes hex string, 
prefixed by '0x'.",
+       optional => 1,
+    },
+);
+
+my $add_throttle_desc = sub {
+    my ($key, $type, $what, $unit, $longunit, $minimum) = @_;
+    my $d = {
+       type => $type,
+       format_description => $unit,
+       description => "Maximum $what in $longunit.",
+       optional => 1,
+    };
+    $d->{minimum} = $minimum if defined($minimum);
+    $drivedesc_base{$key} = $d;
+};
+# throughput: (leaky bucket)
+$add_throttle_desc->('bps',     'integer', 'r/w speed',   'bps',  'bytes per 
second');
+$add_throttle_desc->('bps_rd',  'integer', 'read speed',  'bps',  'bytes per 
second');
+$add_throttle_desc->('bps_wr',  'integer', 'write speed', 'bps',  'bytes per 
second');
+$add_throttle_desc->('mbps',    'number',  'r/w speed',   'mbps', 'megabytes 
per second');
+$add_throttle_desc->('mbps_rd', 'number',  'read speed',  'mbps', 'megabytes 
per second');
+$add_throttle_desc->('mbps_wr', 'number',  'write speed', 'mbps', 'megabytes 
per second');
+$add_throttle_desc->('iops',    'integer', 'r/w I/O',     'iops', 'operations 
per second');
+$add_throttle_desc->('iops_rd', 'integer', 'read I/O',    'iops', 'operations 
per second');
+$add_throttle_desc->('iops_wr', 'integer', 'write I/O',   'iops', 'operations 
per second');
+
+# pools: (pool of IO before throttling starts taking effect)
+$add_throttle_desc->('mbps_max',    'number',  'unthrottled r/w pool',       
'mbps', 'megabytes per second');
+$add_throttle_desc->('mbps_rd_max', 'number',  'unthrottled read pool',      
'mbps', 'megabytes per second');
+$add_throttle_desc->('mbps_wr_max', 'number',  'unthrottled write pool',     
'mbps', 'megabytes per second');
+$add_throttle_desc->('iops_max',    'integer', 'unthrottled r/w I/O pool',   
'iops', 'operations per second');
+$add_throttle_desc->('iops_rd_max', 'integer', 'unthrottled read I/O pool',  
'iops', 'operations per second');
+$add_throttle_desc->('iops_wr_max', 'integer', 'unthrottled write I/O pool', 
'iops', 'operations per second');
+
+# burst lengths
+$add_throttle_desc->('bps_max_length',     'integer', 'length of I/O bursts',  
     'seconds', 'seconds', 1);
+$add_throttle_desc->('bps_rd_max_length',  'integer', 'length of read I/O 
bursts',  'seconds', 'seconds', 1);
+$add_throttle_desc->('bps_wr_max_length',  'integer', 'length of write I/O 
bursts', 'seconds', 'seconds', 1);
+$add_throttle_desc->('iops_max_length',    'integer', 'length of I/O bursts',  
     'seconds', 'seconds', 1);
+$add_throttle_desc->('iops_rd_max_length', 'integer', 'length of read I/O 
bursts',  'seconds', 'seconds', 1);
+$add_throttle_desc->('iops_wr_max_length', 'integer', 'length of write I/O 
bursts', 'seconds', 'seconds', 1);
+
+# legacy support
+$drivedesc_base{'bps_rd_length'} = { alias => 'bps_rd_max_length' };
+$drivedesc_base{'bps_wr_length'} = { alias => 'bps_wr_max_length' };
+$drivedesc_base{'iops_rd_length'} = { alias => 'iops_rd_max_length' };
+$drivedesc_base{'iops_wr_length'} = { alias => 'iops_wr_max_length' };
+
+my $ide_fmt = {
+    %drivedesc_base,
+    %model_fmt,
+    %ssd_fmt,
+    %wwn_fmt,
+};
+PVE::JSONSchema::register_format("pve-qm-ide", $ide_fmt);
+
+my $idedesc = {
+    optional => 1,
+    type => 'string', format => $ide_fmt,
+    description => "Use volume as IDE hard disk or CD-ROM (n is 0 to " .($MAX_IDE_DISKS 
-1) . ").",
+};
+PVE::JSONSchema::register_standard_option("pve-qm-ide", $idedesc);
+
+my $scsi_fmt = {
+    %drivedesc_base,
+    %iothread_fmt,
+    %queues_fmt,
+    %scsiblock_fmt,
+    %ssd_fmt,
+    %wwn_fmt,
+};
+my $scsidesc = {
+    optional => 1,
+    type => 'string', format => $scsi_fmt,
+    description => "Use volume as SCSI hard disk or CD-ROM (n is 0 to " . 
($MAX_SCSI_DISKS - 1) . ").",
+};
+PVE::JSONSchema::register_standard_option("pve-qm-scsi", $scsidesc);
+
+my $sata_fmt = {
+    %drivedesc_base,
+    %ssd_fmt,
+    %wwn_fmt,
+};
+my $satadesc = {
+    optional => 1,
+    type => 'string', format => $sata_fmt,
+    description => "Use volume as SATA hard disk or CD-ROM (n is 0 to " . 
($MAX_SATA_DISKS - 1). ").",
+};
+PVE::JSONSchema::register_standard_option("pve-qm-sata", $satadesc);
+
+my $virtio_fmt = {
+    %drivedesc_base,
+    %iothread_fmt,
+};
+my $virtiodesc = {
+    optional => 1,
+    type => 'string', format => $virtio_fmt,
+    description => "Use volume as VIRTIO hard disk (n is 0 to " . ($MAX_VIRTIO_DISKS - 
1) . ").",
+};
+PVE::JSONSchema::register_standard_option("pve-qm-virtio", $virtiodesc);
+
+my $alldrive_fmt = {
+    %drivedesc_base,
+    %iothread_fmt,
+    %model_fmt,
+    %queues_fmt,
+    %scsiblock_fmt,
+    %ssd_fmt,
+    %wwn_fmt,
+};
+
+my $efidisk_fmt = {
+    volume => { alias => 'file' },
+    file => {
+       type => 'string',
+       format => 'pve-volume-id-or-qm-path',
+       default_key => 1,
+       format_description => 'volume',
+       description => "The drive's backing volume.",
+    },
+    format => get_standard_option('pve-qm-image-format'),
+    size => {
+       type => 'string',
+       format => 'disk-size',
+       format_description => 'DiskSize',
+       description => "Disk size. This is purely informational and has no 
effect.",
+       optional => 1,
+    },
+};
+
+my $efidisk_desc = {
+    optional => 1,
+    type => 'string', format => $efidisk_fmt,
+    description => "Configure a Disk for storing EFI vars",
+};
+
+PVE::JSONSchema::register_standard_option("pve-qm-efidisk", $efidisk_desc);
+
+for (my $i = 0; $i < $MAX_IDE_DISKS; $i++)  {
+    $drivedesc_hash->{"ide$i"} = $idedesc;
+}
+
+for (my $i = 0; $i < $MAX_SATA_DISKS; $i++)  {
+    $drivedesc_hash->{"sata$i"} = $satadesc;
+}
+
+for (my $i = 0; $i < $MAX_SCSI_DISKS; $i++)  {
+    $drivedesc_hash->{"scsi$i"} = $scsidesc;
+}
+
+for (my $i = 0; $i < $MAX_VIRTIO_DISKS; $i++)  {
+    $drivedesc_hash->{"virtio$i"} = $virtiodesc;
+}
+
+$drivedesc_hash->{efidisk0} = $efidisk_desc;
+
+sub valid_drive_names {
+    # order is important - used to autoselect boot disk
+    return ((map { "ide$_" } (0 .. ($MAX_IDE_DISKS - 1))),
+            (map { "scsi$_" } (0 .. ($MAX_SCSI_DISKS - 1))),
+            (map { "virtio$_" } (0 .. ($MAX_VIRTIO_DISKS - 1))),
+            (map { "sata$_" } (0 .. ($MAX_SATA_DISKS - 1))),
+            'efidisk0');
+}
+
+sub is_valid_drivename {
+    my $dev = shift;
+
+    return defined($drivedesc_hash->{$dev});
+}
+
+# ideX = [volume=]volume-id[,media=d][,cyls=c,heads=h,secs=s[,trans=t]]
+#        [,snapshot=on|off][,cache=on|off][,format=f][,backup=yes|no]
+#        [,rerror=ignore|report|stop][,werror=enospc|ignore|report|stop]
+#        [,aio=native|threads][,discard=ignore|on][,detect_zeroes=on|off]
+#        [,iothread=on][,serial=serial][,model=model]
+
+sub parse_drive {
+    my ($key, $data) = @_;
+
+    my ($interface, $index);
+
+    if ($key =~ m/^([^\d]+)(\d+)$/) {
+       $interface = $1;
+       $index = $2;
+    } else {
+       return undef;
+    }
+
+    my $desc = $key =~ /^unused\d+$/ ? $alldrive_fmt
+                                     : $drivedesc_hash->{$key}->{format};
+    if (!$desc) {
+       warn "invalid drive key: $key\n";
+       return undef;
+    }
+    my $res = eval { PVE::JSONSchema::parse_property_string($desc, $data) };
+    return undef if !$res;
+    $res->{interface} = $interface;
+    $res->{index} = $index;
+
+    my $error = 0;
+    foreach my $opt (qw(bps bps_rd bps_wr)) {
+       if (my $bps = defined(delete $res->{$opt})) {
+           if (defined($res->{"m$opt"})) {
+               warn "both $opt and m$opt specified\n";
+               ++$error;
+               next;
+           }
+           $res->{"m$opt"} = sprintf("%.3f", $bps / (1024*1024.0));
+       }
+    }
+
+    # can't use the schema's 'requires' because of the mbps* => bps* "transforming 
aliases"
+    for my $requirement (
+       [mbps_max => 'mbps'],
+       [mbps_rd_max => 'mbps_rd'],
+       [mbps_wr_max => 'mbps_wr'],
+       [miops_max => 'miops'],
+       [miops_rd_max => 'miops_rd'],
+       [miops_wr_max => 'miops_wr'],
+       [bps_max_length => 'mbps_max'],
+       [bps_rd_max_length => 'mbps_rd_max'],
+       [bps_wr_max_length => 'mbps_wr_max'],
+       [iops_max_length => 'iops_max'],
+       [iops_rd_max_length => 'iops_rd_max'],
+       [iops_wr_max_length => 'iops_wr_max']) {
+       my ($option, $requires) = @$requirement;
+       if ($res->{$option} && !$res->{$requires}) {
+           warn "$option requires $requires\n";
+           ++$error;
+       }
+    }
+
+    return undef if $error;
+
+    return undef if $res->{mbps_rd} && $res->{mbps};
+    return undef if $res->{mbps_wr} && $res->{mbps};
+    return undef if $res->{iops_rd} && $res->{iops};
+    return undef if $res->{iops_wr} && $res->{iops};
+
+    if ($res->{media} && ($res->{media} eq 'cdrom')) {
+       return undef if $res->{snapshot} || $res->{trans} || $res->{format};
+       return undef if $res->{heads} || $res->{secs} || $res->{cyls};
+       return undef if $res->{interface} eq 'virtio';
+    }
+
+    if (my $size = $res->{size}) {
+       return undef if !defined($res->{size} = 
PVE::JSONSchema::parse_size($size));
+    }
+
+    return $res;
+}
+
+sub print_drive {
+    my ($drive) = @_;
+    my $skip = [ 'index', 'interface' ];
+    return PVE::JSONSchema::print_property_string($drive, $alldrive_fmt, 
$skip);
+}
+
+sub foreach_drive {
+    my ($conf, $func, @param) = @_;
+
+    foreach my $ds (valid_drive_names()) {
+       next if !defined($conf->{$ds});
+
+       my $drive = parse_drive($ds, $conf->{$ds});
+       next if !$drive;
+
+       &$func($ds, $drive, @param);
+    }
+}
+
+1;
diff --git a/PVE/QemuServer/Makefile b/PVE/QemuServer/Makefile
index 6a49626..fd8cfbb 100644
--- a/PVE/QemuServer/Makefile
+++ b/PVE/QemuServer/Makefile
@@ -9,6 +9,7 @@ SOURCES=PCI.pm          \
        Monitor.pm      \
        Machine.pm      \
        CPUConfig.pm    \
+       Drive.pm        \
.PHONY: install
  install: ${SOURCES}
--
2.20.1


_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel



_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Reply via email to