Feature hostname by frankiejol · Pull Request #2353 · UPC/ravada

frankiejol · 2026-07-16T10:22:35Z

Resolve the host name at the display connection fields

closes #2338

issue #2338

Copilot

Pull request overview

This PR addresses #2338 by resolving and surfacing a hostname (instead of an IP) in display connection information, so clients (notably RDP with SSL certificates) can use a certificate-valid name.

Changes:

Add hostname resolution + caching for display endpoints and use it when generating RDP/SPICE connection files.
Expand/adjust VM display tests to validate hostname propagation into stored display data and connection files.
Minor reliability fixes in port/machine/cleanup logic and test helpers.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
t/vm/n20_name.t	Avoids /tmp display config filename collisions by making it unique per test run.
t/vm/94_ports_exposed.t	Updates expected default value for `ports_exposed` to match current behavior.
t/vm/23_display.t	Adds hostname-related display assertions and connection-file validation for multiple display drivers.
t/mojo/10_login.t	Improves failure clarity when a domain cannot be found (croak with message).
t/lib/Test/Ravada.pm	Adjusts cleanup/discovery behavior when removing old domains/requests.
lib/Ravada/VM.pm	Tweaks used-port listing logic (notably for display TLS ports).
lib/Ravada/Domain/KVM.pm	Adds a guard around machine-type parsing in migration XML normalization.
lib/Ravada/Domain.pm	Implements hostname resolution + caching and uses hostname in display info + RDP/SPICE file generation.
lib/Ravada.pm	Refactors exposed-port cleanup logic to update per-row and retry on conflicts.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+    my ($in, $out, $err);
+    run3(['host',$ip], \$in, \$out, \$err);
+    my ($hostname) = $out =~ /pointer (.*)\./;
+    if (!$hostname) {
+        warn "I can't fetch hostname from $out" if !$hostname;
+        $hostname = $ip;
+    }


+    my $name = gethostbyaddr($ip, AF_INET);
+    return $name if $name && $name !~ /\d+\.\d+\.\d+/;
+
+    my ($in, $out, $err);
+    run3(['host',$ip], \$in, \$out, \$err);
+    my ($hostname) = $out =~ /pointer (.*)\./;
+    ($hostname) = $out =~ /pointer (.*)\./;
+    die "I can't fetch hostname from $out" if !$hostname;
+


issue #2338

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.

    my @reqs;
    for my $machine ( @$machines, @machines2) {
        next unless $machine->{name} =~ /$base_name/;
-        remove_domain_and_clones_req($machine,$wait);
+        remove_domain_and_clones_req($machine,$wait,0);
    }


+sub _get_hostname($ip) {
+
+    return $ip if $ip !~ /^\d+\.\d+\.\d+\.\d+$/;
+
+    my ($in, $out, $err);
+    run3(['host',$ip], \$in, \$out, \$err);
+    my ($hostname) = $out =~ /pointer (.*)\./;
+    ($hostname) = $out =~ /pointer (.*)\./;
+    die "I can't fetch hostname from $out" if !$hostname;
+
+    return $hostname;
+}


+    my ($in, $out, $err);
+    run3(['host',$ip], \$in, \$out, \$err);
+    my ($hostname) = $out =~ /pointer (.*)\./;
+    if (!$hostname) {
+        warn "I can't fetch hostname from $out";
+        $hostname = $ip;
+    }
+    $CACHE_HOSTNAME{$ip} = $hostname;
+
+    return $hostname;
+}


Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

issue #2338

…ature/2338_hostname

Copilot

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.

 sub remove_old_domains_req($wait=1, $run_request=0) {
    my $base_name = base_domain_name();
-    _discover();
+    delete_request( 'enforce_limits', 'set_time', 'refresh_vm' );
+    _discover() if $discover;
    my $machines = rvd_front->list_machines(user_admin);


+    if (!exists $display_new{hostname}) {
+        unlock_hash(%display_new);
+        $display_new{hostname} = $self->_cache_dns($display_new{ip});
+        lock_hash(%display_new);
+    }


+    return $ip if $ip !~ /^\d+\.\d+\.\d+\.\d+$/;
+
+    return $CACHE_HOSTNAME{$ip} if exists $CACHE_HOSTNAME{$ip};
+
+    my $packed_ip = inet_aton($ip);
+    my $name = $packed_ip ? gethostbyaddr($packed_ip, AF_INET) : undef;
+    if ( $name && $name !~ /\d+\.\d+\.\d+/ ) {
+        $CACHE_HOSTNAME{$ip} = $name;
+        return $name;
+    }
+
+    my ($in, $out, $err);
+    run3(['host',$ip], \$in, \$out, \$err);
+    my ($hostname) = $out =~ /pointer (.*)\./;
+    if (!$hostname) {
+        warn "I can't fetch hostname from $out";
+        $hostname = $ip;
+    }
+    $CACHE_HOSTNAME{$ip} = $hostname;
+
+    return $hostname;
+}


+sub _get_hostname($ip) {
+
+    return $ip if $ip !~ /^\d+\.\d+\.\d+\.\d+$/;
+
+    my ($in, $out, $err);
+    run3(['host',$ip], \$in, \$out, \$err);
+    my ($hostname) = $out =~ /pointer (.*)\./;
+    ($hostname) = $out =~ /pointer (.*)\./;
+    die "I can't fetch hostname from $out" if !$hostname;
+
+    return $hostname;
+}


issue #2338

Copilot

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Suppressed comments (5)

lib/Ravada/Domain.pm:1478

_get_hostname() shells out to the host command without any timeout/limit. If DNS is slow or misconfigured this can block request processing (it’s called from _store_display/info). Consider calling host with an explicit timeout and querying only PTR to keep lookups bounded.

    if ( ! $name || $name =~ /\d+\.\d+\.\d+/ ) {
        my ($in, $out, $err);
        run3(['host',$ip], \$in, \$out, \$err);
        ($name) = $out =~ /pointer (.*)\./;
    }

lib/Ravada/Domain.pm:1446

Hostname is only populated when the field is missing, so if the display IP changes (e.g. NAT/public IP change) an existing stored hostname can become stale and then be reused when generating display files. Consider recomputing the hostname when it's empty or when the IP has changed compared to the stored display row.

    if (!exists $display_new{hostname}) {
        unlock_hash(%display_new);
        $display_new{hostname} = $self->_cache_dns($display_new{ip});
        lock_hash(%display_new);
    }

t/vm/23_display.t:180

The test helper _get_hostname() hard-fails when reverse DNS isn’t available (die "I can't fetch hostname"). That makes the test suite environment-dependent. It would be more robust to fall back to the IP (matching the production logic) and optionally use gethostbyaddr first to avoid the external host dependency.

sub _get_hostname($ip) {

    return $ip if $ip !~ /^\d+\.\d+\.\d+\.\d+$/;

    my ($in, $out, $err);
    run3(['host',$ip], \$in, \$out, \$err);
    my ($hostname) = $out =~ /pointer (.*)\./;
    die "I can't fetch hostname from $out" if !$hostname;

lib/Ravada/Domain.pm:2445

This adds a hostname field, but the UI templates still render display.ip (e.g. templates/main/machine_displays.html.ep and templates/main/run_request.html.ep). As-is, the user-visible “Display IP”/connection fields will likely continue showing the IP, which doesn’t fully satisfy issue #2338. Either update the front-end to prefer display.hostname when present, or have the API return the resolved hostname in the field the UI already uses.

            my @display = $self->display_info($user);
            $info->{display} = $display[0];
            $info->{display}->{hostname} = _get_hostname($info->{display}->{ip})
                if $info->{display}->{ip};

t/vm/23_display.t:143

Using or exit in a test aborts the entire test file (and can hide later failures). Prefer letting the assertion report normally (or die with context) instead of exiting the test runner early.

This issue also appears on line 172 of the same file.

    is($address, _get_hostname($domain->_vm->ip)) or exit;

+sub _cache_dns($self, $ip) {
+    my $name = $self->{_dns}->{$ip};
+    return $name if defined $name;
+
+    $name = _get_hostname($ip);
+
+    warn "Error: name for $ip not found" if !$name;
+
+    $self->{_dns}->{$ip} = $name;
+    return $name;
+}


frankiejol and others added 8 commits June 11, 2026 12:13

wip: show hostname on displays

a407d8c

issue #2338

wip: get hostname and better handle conflicts

772b0bd

issue #2338

wip: fetch host name

b808793

wip: wait for ip to test

c3f9327

issue #2338

wip: return name if no need to resolve

3e34d70

issue #2338

wip: return ip if not resolved

e727be7

issue #2338

wip: cache _hostname

c98709e

issue #2338

wip: removed warning

9207345

issue #2338

frankiejol added this to the v2.4.7 milestone Jul 16, 2026

Copilot AI review requested due to automatic review settings July 16, 2026 10:22

Copilot started reviewing on behalf of frankiejol July 16, 2026 10:23 View session

Copilot AI reviewed Jul 16, 2026

View reviewed changes

frankiejol and others added 3 commits July 16, 2026 13:26

wip: proper call to gethostbyaddr

4c2978c

wip: cache resolved name

0819b33

issue #2338

Potential fix for pull request finding

78c382e

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

frankiejol requested a review from Copilot July 16, 2026 11:36

Copilot started reviewing on behalf of frankiejol July 16, 2026 11:37 View session

Copilot AI reviewed Jul 16, 2026

View reviewed changes

frankiejol and others added 2 commits July 16, 2026 15:52

wip: depends on bind9-host

9525b2c

Potential fix for pull request finding

d442a60

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

frankiejol requested a review from Copilot July 16, 2026 13:53

Copilot started reviewing on behalf of frankiejol July 16, 2026 13:54 View session

frankiejol added 2 commits July 16, 2026 15:57

wip: discover always

f8fa524

issue #2338

Merge branch 'feature/2338_hostname' of github.com:UPC/ravada into fe…

baca87f

…ature/2338_hostname

Copilot AI reviewed Jul 16, 2026

View reviewed changes

frankiejol added 2 commits July 24, 2026 16:45

wip: sanitize resolved hostname

9969200

issue #2338

Merge branch 'main' into feature/2338_hostname

9f35638

frankiejol requested a review from Copilot August 3, 2026 06:57

Copilot started reviewing on behalf of frankiejol August 3, 2026 06:57 View session

Copilot AI reviewed Aug 3, 2026

View reviewed changes

Comment thread lib/Ravada/Domain.pm

Comment on lines +1454 to +1464

sub _cache_dns($self, $ip) {

my $name = $self->{_dns}->{$ip};

return $name if defined $name;

$name = _get_hostname($ip);

warn "Error: name for $ip not found" if !$name;

$self->{_dns}->{$ip} = $name;

return $name;

}

Navigation Menu

Uh oh!

Feature hostname - #2353

Feature hostname#2353
frankiejol wants to merge 17 commits into
mainfrom
feature/2338_hostname

frankiejol commented Jul 16, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

frankiejol commented Jul 16, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants