Fix spice remove

lib/Ravada/Domain.pm

@@ -6445,6 +6445,10 @@ sub _add_hardware_disk($orig, $self, $index, $data) {
 sub _around_add_hardware($orig, $self, $hardware, $index, $data=undef) {
     confess "Error: minimal add hardware index>=0 , got '$index'" if defined $index && $index <0;
 
+
+    die "Error: Virtual Machines with host devices can not be modified while running"
+    if $self->is_active && $self->list_host_devices_locked;
+
     my $data_orig = undef;
     $data_orig = dclone($data ) if ref($data);
 
@@ -6478,6 +6482,9 @@ sub _delete_db_display_by_driver($self, $driver) {
 sub _around_remove_hardware($orig, $self, $hardware, $index=undef, $options=undef) {
     confess "Error: supply either index or options when removing hardware " if !defined $index && !defined $options;
 
+    die "Error: Virtual Machines with host devices can not be modified while running"
+    if $self->is_active && $self->list_host_devices_locked;
+
     my $id_filesystem;
     if ( $hardware eq 'filesystem') {
         my @fs = $self->get_controller('filesystem');

lib/Ravada/Domain/KVM.pm

@@ -2537,8 +2537,8 @@ sub _set_controller_display_spice($self, $number, $data) {
             $graphic = $g0;
         }
     }
+    my ($devices) = $doc->findnodes("/domain/devices");
     if (!$graphic) {
-        my ($devices) = $doc->findnodes("/domain/devices");
         $graphic = $devices->addNewChild(undef,'graphics');
     }
 
@@ -2569,6 +2569,8 @@ sub _set_controller_display_spice($self, $number, $data) {
         my $item = $graphic->addNewChild(undef, $name);
         $item->setAttribute($attrib => $value);
     }
+    _add_spice_related($devices);
+
     $self->reload_config($doc);
 }
 
@@ -2691,6 +2693,13 @@ sub _remove_device($self, $index, $device, $attribute_name0=undef, $attribute_va
             $file = $source->getAttribute('file') if $source;
 
             $devices->removeChild($controller);
+
+            if ($device eq 'graphics' && $controller->getAttribute('type') eq 'spice') {
+                $self->_remove_spice_related($devices);
+            } elsif ($device eq 'video') {
+                $self->_set_minimal_video($devices);
+            }
+
             $self->_vm->connect if !$self->_vm->vm;
             $self->reload_config($doc);
 
@@ -2706,6 +2715,65 @@ sub _remove_device($self, $index, $device, $attribute_name0=undef, $attribute_va
         ." not removed, only ".($ind)." found in ".$self->name."\n";
 }
 
+sub _set_minimal_video($self, $devices) {
+    my $string = 'video';
+    my @current = $devices->findnodes($string);
+    if (!@current) {
+        my $video = $devices->addNewChild(undef,'video');
+        my $model = $video->addNewChild(undef,'model');
+        $model->setAttribute('type' => 'none');
+    }
+
+}
+
+sub _remove_spice_related($self, $devices) {
+    for my $redir ($devices->findnodes("redirdev[\@type=\'spicevmc\']")) {
+        $devices->removeChild($redir);
+    }
+    my ($audio) = $devices->findnodes("audio[\@type='spice']");
+    $devices->removeChild($audio) if $audio;
+
+    my ($channel) = $devices->findnodes("channel[\@type='spicevmc']");
+    $devices->removeChild($channel) if $channel;
+}
+
+sub _add_spice_related($devices) {
+
+    if (!$devices->findnodes("redirdev[\@type=\'spicevmc\']")) {
+        for ( 1 .. 3 ) {
+            my $redir = $devices->addNewChild(undef,'redirdev');
+            $redir->setAttribute('type' => 'spicevmc');
+            $redir->setAttribute('bus' => 'usb');
+        }
+    }
+    my @audio = $devices->findnodes("audio");
+    my ($none) = grep { $_->getAttribute('type') eq 'none' } @audio;
+
+    if (!@audio || $none) {
+        $devices->removeChild($none) if $none;
+
+        my $audio= $devices->addNewChild(undef,'audio');
+        $audio->setAttribute(type =>'spice');
+        $audio->setAttribute(id => 1);
+    }
+
+    if (!$devices->findnodes("channel[\@type=\'spicevmc\']")) {
+        my $channel = $devices->addNewChild(undef,'channel');
+        $channel->setAttribute('type' => 'spicevmc');
+
+        my $target = $channel->addNewChild(undef,'target');
+        $target->setAttribute('type' => 'virtio');
+        $target->setAttribute('name' => 'com.redhat.spice.0');
+
+        my $address = $channel->addNewChild(undef,'address');
+        $address->setAttribute('type' => 'virtio-serial');
+        $address->setAttribute('controller' => 0);
+        $address->setAttribute('bus' => 0);
+        $address->setAttribute('port' => 1);
+
+    }
+}
+
 sub _remove_controller_display($self, $index, $attribute_name=undef, $attribute_value=undef) {
     $self->_remove_device($index,'graphics', $attribute_name,$attribute_value );
 }

public/js/ravada.js

@@ -940,6 +940,10 @@
                 }
 
             }
+            if(typeof($scope.showmachine.hardware[hardware][index]["_index"])
+                !== 'undefined') {
+                index=$scope.showmachine.hardware[hardware][index]._index;
+            }
             $scope.request('remove_hardware',{
                     'id_domain': $scope.showmachine.id
                      ,'name': hardware

t/kvm/s10_spice.t

@@ -6,6 +6,9 @@ use Data::Dumper;
 use IPC::Run3;
 use Test::More;
 
+use feature qw(signatures);
+no warnings "experimental::signatures";
+
 use lib 't/lib';
 use Test::Ravada;
 
@@ -15,7 +18,7 @@ init();
 my @VMS = vm_names();
 my $USER = create_user("foo","bar", 1);
 
-my $TLS;
+my $TLS=0;
 
 #######################################################
 
@@ -47,8 +50,106 @@ sub test_spice {
     my ($port_f) = $display_file =~ m{port=(.*)}mx;
     is($ip_d, $ip_f);
     is($port_d, $port_f);
+    return $domain;
 }
 
+sub _remove_display($domain) {
+
+    my $info = $domain->info(user_admin);
+    my $index;
+    my $index_tls;
+    my $n = 0;
+    for my $item ( @{$info->{hardware}->{display}} ) {
+        if ( $item->{driver} eq 'spice' ) {
+            $index=$item->{_index};
+        }
+        if ( $item->{driver} eq 'spice-tls' ) {
+            $index_tls=$n;
+        }
+        last if $index && $index_tls;
+        $n++;
+    }
+
+    ok(defined $index) or die "No spice found in hardwre"
+    .Dumper([ map { $_->{driver} } @{$info->{hardware}->{display}}] );
+
+
+    ok(defined $index_tls) or die "No spice-tls found in hardwre"
+    .Dumper([ map { $_->{driver} } @{$info->{hardware}->{display}}] );
+
+    my $req = Ravada::Request->remove_hardware(
+        uid => user_admin->id
+        ,id_domain => $domain->id
+        ,name => 'display'
+        ,index => $index
+    );
+    wait_request();
+
+    is($req->error,'');
+    Ravada::Request->force_shutdown(
+        uid => user_admin->id
+        ,id_domain => $domain->id
+    );
+    wait_request();
+
+    my $doc =XML::LibXML->load_xml(string => $domain->domain->get_xml_description());
+    my ($spice) = $doc->findnodes("/domain/devices/graphics");
+    ok(!$spice);
+
+    $info = $domain->info(user_admin);
+    my ($hw_spice) = grep { $_->{driver} =~ /spice/ } @{$info->{hardware}->{display}};
+    ok(!$hw_spice);
+}
+
+sub _add_display($domain,$driver='spice') {
+    my $req = Ravada::Request->add_hardware(
+        uid => user_admin->id
+        ,id_domain => $domain->id
+        ,name => 'display'
+        ,data => { driver => $driver}
+    );
+    wait_request();
+
+    my $doc =XML::LibXML->load_xml(string => $domain->domain->get_xml_description());
+    my ($spice) = $doc->findnodes("/domain/devices/graphics[\@type='$driver']");
+    ok($spice);
+
+    my @redir = $doc->findnodes("/domain/devices/redirdev[\@type=\'spicevmc\']");
+    ok(scalar(@redir)>2);
+
+    my @audio = $doc->findnodes("/domain/devices/audio");
+    ok(scalar(@audio));
+
+    my ($none) = grep { $_->getAttribute('type') eq 'none' } @audio;
+    ok(!$none,"Expecting no 'none' audio");
+
+    my @audio_spice = $doc->findnodes("/domain/devices/audio[\@type='spice']");
+    is(scalar(@audio_spice),1);
+
+    my @channel = $doc->findnodes("/domain/devices/channel[\@type='spicevmc']");
+    is(scalar(@channel),1);
+}
+
+sub test_remove_spice($domain) {
+    Ravada::Request->start_domain(
+        uid => user_admin->id
+        ,id_domain => $domain->id
+    );
+    wait_request();
+
+    my $doc =XML::LibXML->load_xml(string => $domain->domain->get_xml_description());
+    my ($spice) = $doc->findnodes("/domain/devices/graphics");
+    die "Error: no spice found in ".$domain->name if !$spice;
+
+    _remove_display($domain);
+
+    _add_display($domain, 'spice');
+
+    $domain->start(user_admin);
+
+    my $info = $domain->info(user_admin);
+    like($info->{hardware}->{display}->[0]->{driver},qr'spice');
+}
 
 #######################################################
 
@@ -76,7 +177,8 @@ SKIP: {
 
     $TLS = 1 if check_libvirt_tls() && $vm_name eq 'KVM';
 
-    test_spice($vm_name);
+    my $domain = test_spice($vm_name);
+    test_remove_spice($domain);
 }
 
 end();

t/nodes/60_hardware.t

@@ -225,7 +225,7 @@ sub test_change_hardware($vm, @nodes) {
     }
     my @hardware = grep (!/^disk$/, sort keys %{$info->{hardware}});
     push @hardware,("disk");
-    for my $hardware ( @hardware) {
+    for my $hardware (reverse @hardware) {
         next if $hardware =~ /cpu|features|memory/;
         my $tls = 0;
         $tls = grep {$_->{driver} =~ /-tls/} @{$info->{hardware}->{$hardware}}
@@ -260,9 +260,14 @@ sub test_change_hardware($vm, @nodes) {
 
             my $info2 = $clone2->info(user_admin);
             my $devices2 = $info2->{hardware}->{$hardware};
-            is( scalar(@$devices2),$n_expected
+            if ($hardware eq 'video' && $vm->type eq 'KVM') {
+                is( scalar(@$devices2),1);
+                is($devices2->[0]->{type},'none');
+            } else {
+                is( scalar(@$devices2),$n_expected
                 , $clone2->name.": Expecting 1 $hardware device less in instance in node ".$node->name)
                 or die Dumper($devices2);
+            }
         }
 
         my $clone_fresh = Ravada::Domain->open($clone->id);

t/vm/20_base.t

@@ -523,7 +523,7 @@ sub test_display_info($vm) {
     my $exp_driver = 'spice';
     $exp_driver .= "-tls" if $TLS;
     is($display_h->[0]->{driver}, $exp_driver) if $domain->type eq 'KVM';
-    like($display_h->[0]->{password},qr{..+}, $domain_f->name) or exit if $domain->type eq 'KVM';
+    like($display_h->[0]->{password},qr{..+}, $domain_f->name) or confess Dumper($display_h->[0]) if $domain->type eq 'KVM';
     is($display_h->[0]->{id_exposed_port},undef); # spice doesn't need exposed port
 
     is($display_h->[1+$TLS]->{driver}, 'rdp');
@@ -1481,7 +1481,7 @@ sub test_display_drivers($vm, $remove) {
         Ravada::Request->start_domain(uid => user_admin->id
             ,id_domain => $domain->id
         );
-        for ( 1 .. 10 ) {
+        for my $n ( 1 .. 20 ) {
             wait_request(debug => 0);
             last if !$req->error || $req->error !~ /Retry/i;
             sleep 1;
@@ -1608,10 +1608,11 @@ sub test_display_conflict($vm) {
     for ( 1 .. 10 ) {
         $port3 = $domain->exposed_port(22);
         last if $port3->{public_port} && $port3->{public_port} != $display_builtin->{port};
-        Ravada::Request->refresh_machine(uid => user_admin->id ,id_domain => $domain->id);
+        Ravada::Request->refresh_machine(uid => user_admin->id ,id_domain => $domain->id
+            , _force => 1);
         wait_request(debug => 0);
     }
-    isnt($port3->{public_port},$display_builtin->{port}) or die;
+    isnt($port3->{public_port},$display_builtin->{port}) or die $domain->id." ".$domain->name;
 
     $domain->remove(user_admin);