On 9/18/18 10:19 AM, Dominik Csapak wrote:
> this adds a VM Generation ID device uses by Windows (Server) to determine
> some specific actions that may have happened with the vm
> such as rollback, restore, etc.
>
shouldn't this be handled more like the MAC from a NIC?
I.e., explicitly generate one when needed, not doing magic auto stuff?
You could do this automatically for all VMs, while it comes from
windows it's intended to be guest OS agnostic and is exposed over
fw_cfg/ACPI, AFAIS.[0]
Maybe add none if there's any "hide that we virtualize" flag is on,
but else I do not really see a point in not doing this?
(allowing to let the user disable it with '0', seems nonetheless
a possible reasonable option)
> see:
>
> https://docs.microsoft.com/en-us/windows/desktop/hyperv_v2/virtual-machine-generation-identifier
>
> for details on how it works and when it should change
>
> Signed-off-by: Dominik Csapak
> ---
> PVE/API2/Qemu.pm | 5 +
> PVE/QemuConfig.pm | 4
> PVE/QemuServer.pm | 23 +++
> PVE/QemuServer/Makefile | 1 +
> PVE/QemuServer/VMGenID.pm | 29 +
> 5 files changed, 62 insertions(+)
> create mode 100644 PVE/QemuServer/VMGenID.pm
>
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index c1cc01b..6ac3110 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -2732,6 +2732,11 @@ __PACKAGE__->register_method({
> $smbios1->{uuid} = $uuid_str;
> $newconf->{smbios1} = PVE::QemuServer::print_smbios1($smbios1);
>
> + # auto generate a new vmgenid
> + if ($oldconf->{vmgenid}) {
> + $newconf->{vmgenid} = 'auto';
why not actually just generate one here?
This seems to just unnecessarily add another in-between step?
As QEMU allows to use auto for this, auto-generating a new vmgenid at each
start, but you also detect auto and generate one here this results in
possible confusion/intransparency about who the generator is, at least when
looking at the code.
Also an user could think this gets passed directly to qemu and be confused too.
> + }
> +
> delete $newconf->{template};
>
> if ($param->{name}) {
> diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
> index cd116bd..5a05c1b 100644
> --- a/PVE/QemuConfig.pm
> +++ b/PVE/QemuConfig.pm
> @@ -300,6 +300,10 @@ sub __snapshot_rollback_hook {
> # in the original config.
> delete $conf->{machine} if $snap->{vmstate} &&
> !defined($data->{oldmachine});
> }
> +
> + if (defined($conf->{vmgenid})) {
> + $conf->{vmgenid} = 'auto';
> + }
> }
>
> return;
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index af0631d..1b68ddd 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -34,6 +34,7 @@ use PVE::QemuServer::PCI qw(print_pci_addr print_pcie_addr);
> use PVE::QemuServer::Memory;
> use PVE::QemuServer::USB qw(parse_usb_device);
> use PVE::QemuServer::Cloudinit;
> +use PVE::QemuServer::VMGenID;
> use PVE::Systemd;
> use Time::HiRes qw(gettimeofday);
> use File::Copy qw(copy);
> @@ -559,6 +560,12 @@ EODESCR
> description => "Select BIOS implementation.",
> default => 'seabios',
> },
> +vmgenid => {
> + type => 'string',
> + pattern =>
> '(?:auto)|(?:[a-fA-F0-9]{8}(?:-[a-fA-F0-9]{4}){3}-[a-fA-F0-9]{12})',
> +description => "The VM Generation ID. Use the special value 'auto'
> to autogenerate one",
> + optional => 1,
> +},
> };
>
> my $confdesc_cloudinit = {
> @@ -3191,6 +3198,10 @@ sub config_to_command {
> push @$cmd, '-smbios', "type=1,$conf->{smbios1}";
> }
>
> +if ($conf->{vmgenid}) {
regarding this and ... (below)
> + push @$devices,
> PVE::QemuServer::VMGenID::create_device($conf->{vmgenid}, 0);
> +}
> +
> if ($conf->{bios} && $conf->{bios} eq 'ovmf') {
> die "uefi base image not found\n" if ! -f $OVMF_CODE;
>
> @@ -4593,6 +4604,14 @@ sub vmconfig_apply_pending {
> }
> }
>
> +sub vmconfig_apply_vmgenid_generation {
> +my ($vmid, $conf) = @_;
> +if ($conf->{vmgenid}) {
> + $conf->{vmgenid} =
> PVE::QemuServer::VMGenID::create_or_parse($conf->{vmgenid});
.. this: why not do the actual work here? This seems to just add indirection
for the sake of it, netto adding more line and rabbit holes to jump in?
(see also below at adding the new module diff)
> +}
> +PVE::QemuConfig->write_config($vmid, $conf);
> +}
> +
> my $safe_num_ne = sub {
> my ($a, $b) = @_;
>
> @@ -4779,6 +4798,8 @@ sub vm_start {
>
> die "VM $vmid already running\n" if check_running($vmid, undef,
> $migratedfrom);
>
> + vmconfig_apply_vmgenid_generation($vmid, $conf);
> +
> if (!$statefile && scalar(keys %{$conf->{pending}})) {
> vmconfig_apply_pending($vmid, $conf, $storecfg);
> $conf = PVE::QemuConfig->load_config($vmid); # update/reload
> @@ -5554,6 +5575,8 @@ sub
