Re: [pve-devel] [PATCH qemu-server] feature #1027: virtio-9p & virtio-fs support

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On October 7, 2022 4:29 pm, Markus Frank wrote:
> adds support for sharing directorys with a guest vm.
> 
> virtio-9p can be simply started with qemu.
> virtio-fs needs virtiofsd to be started before qemu.
> 
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
> I chose MAX_SHAREDFILES to be 10, because I think it is more than enough.

famous last words ;)

> 
>  PVE/QemuServer.pm | 113 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 113 insertions(+)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 4e85dd0..580133b 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -272,6 +272,31 @@ my $rng_fmt = {
>      },
>  };
>  
> +my $sharedfiles_fmt = {
> +    type => {
> +	type => 'string',
> +	default_key => 1,
> +	enum => ['virtio-9p', 'virtio-fs'],
> +	description => "sharedfiles via"
> +	    ." virtio-9p (https://www.linux-kvm.org/page/9p_virtio)"
> +	    ." or virtio-fs (https://virtio-fs.gitlab.io/howto-qemu.html)",
> +	format_description => "virtio-sharedfiles-type",
> +	optional => 1,
> +    },
> +    path => {
> +	type => 'string',
> +	description => "path you want to share with the guest VM",
> +	format_description => "virtio-sharedfiles-path",
> +	optional => 1,

this should have more restrictions than being an arbitrary string.

ideally we could take this opportunity and implement some sort of 
"defined host source paths" feature that we can re-use for bind mounts 
as well:

- admin defines dirs on the host that are eligibly for mounting into 
  guests
- admin gives access via an ACL
- user can then use this object as bind mount/shared dir source without 
  requiring root access

(and not allow arbitrary paths here at all?)

> +    },
> +    tag => {
> +	type => 'string',
> +	description => "tag name for mounting in the guest VM",
> +	format_description => "virtio-sharedfiles-tag",
> +	optional => 1,
> +    },
> +};
> +
>  my $meta_info_fmt = {
>      'ctime' => {
>  	type => 'integer',
> @@ -826,6 +851,7 @@ while (my ($k, $v) = each %$confdesc) {
>  
>  my $MAX_USB_DEVICES = 5;
>  my $MAX_NETS = 32;
> +my $MAX_SHAREDFILES = 10;
>  my $MAX_SERIAL_PORTS = 4;
>  my $MAX_PARALLEL_PORTS = 3;
>  my $MAX_NUMA = 8;
> @@ -968,6 +994,12 @@ my $netdesc = {
>      description => "Specify network devices.",
>  };
>  
> +my $sharedfilesdesc = {
> +    optional => 1,
> +    type => 'string', format => $sharedfiles_fmt,
> +    description => "share files between host and guest",
> +};
> +
>  PVE::JSONSchema::register_standard_option("pve-qm-net", $netdesc);
>  
>  my $ipconfig_fmt = {
> @@ -1029,6 +1061,10 @@ for (my $i = 0; $i < $MAX_NETS; $i++)  {
>      $confdesc_cloudinit->{"ipconfig$i"} = $ipconfigdesc;
>  }
>  
> +for (my $i = 0; $i < $MAX_SHAREDFILES; $i++)  {
> +    $confdesc->{"sharedfiles$i"} = $sharedfilesdesc;
> +}
> +
>  foreach my $key (keys %$confdesc_cloudinit) {
>      $confdesc->{$key} = $confdesc_cloudinit->{$key};
>  }
> @@ -1933,6 +1969,16 @@ sub parse_net {
>      return $res;
>  }
>  
> +sub parse_sharedfiles {
> +    my ($value) = @_;
> +
> +    return if !$value;
> +    my $res = eval { parse_property_string($sharedfiles_fmt, $value) };
> +
> +    warn $@ if $@;
> +    return $res;
> +}
> +
>  # ipconfigX ip=cidr,gw=ip,ip6=cidr,gw6=ip
>  sub parse_ipconfig {
>      my ($data) = @_;
> @@ -4022,6 +4068,45 @@ sub config_to_command {
>  	push @$devices, '-device', $netdevicefull;
>      }
>  
> +    my $onevirtiofs = 0;
> +    for (my $i = 0; $i < $MAX_SHAREDFILES; $i++) {
> +	my $sharedfilesstr = "sharedfiles$i";
> +
> +	next if !$conf->{$sharedfilesstr};
> +	my $sharedfiles = parse_sharedfiles($conf->{$sharedfilesstr});
> +	next if !$sharedfiles;
> +
> +	die $sharedfilesstr.' needs a type (virtio-9p or virtio-fs)' if !$sharedfiles->{type};
> +	die $sharedfilesstr.' needs a path to a directory to share' if !$sharedfiles->{path};

should check whether path exists and is a directory

> +	die $sharedfilesstr.' needs a mount tag' if !$sharedfiles->{tag};
> +
> +	if ($sharedfiles->{type} eq 'virtio-fs' && $conf->{numa}) {
> +	    die "disable numa to use virtio-fs or use virtio-9p instead";
> +	}
> +
> +	mkdir $sharedfiles->{path};
> +
> +	if ($sharedfiles->{type} eq 'virtio-9p') {
> +	    push @$devices, '-fsdev', 'local,security_model=passthrough,id=fsdev'.$i
> +		.',path='.$sharedfiles->{path};

not sure about the encoding here on the qemu side, but this right now 
allows injecting ',' followed by other fsdev options, so likely we want 
to escape/encode and have more checks on the path.

> +	    push @$devices, '-device', 'virtio-9p-pci,id=fs'.$i.',fsdev=fsdev'.$i
> +		.',mount_tag='.$sharedfiles->{tag};
> +	}
> +	if ($sharedfiles->{type} eq 'virtio-fs') {
> +	    push @$devices, '-chardev', 'socket,id=virtfs'.$i
> +		.',path=/var/run/virtiofsd/vm'.$vmid.'-fs'.$i;
> +	    push @$devices, '-device', 'vhost-user-fs-pci,queue-size=1024,chardev=virtfs'.$i
> +		.',tag='.$sharedfiles->{tag};
> +	    $onevirtiofs = 1;
> +	}
> +    }
> +
> +    if ($onevirtiofs) {
> +	push @$devices, '-object', 'memory-backend-file,id=mem,'
> +	    .'size='.$conf->{memory}.'M,mem-path=/dev/shm,share=on';
> +	push @$devices, '-numa', 'node,memdev=mem';
> +    }
> +
>      if ($conf->{ivshmem}) {
>  	my $ivshmem = parse_property_string($ivshmem_fmt, $conf->{ivshmem});
>  
> @@ -4107,6 +4192,22 @@ sub config_to_command {
>      return wantarray ? ($cmd, $vollist, $spice_port) : $cmd;
>  }
>  
> +sub start_virtiofs {
> +    my ($vmid, $path, $fsid) = @_;
> +    # virtiofsd does not run in background until vhost-user connects
> +    # to the socket, so it has to be started in a fork or with a tool
> +    # like daemonize
> +
> +    my $pid = fork();
> +    if ($pid == 0) {
> +	run_command('/usr/lib/kvm/virtiofsd --daemonize --socket-path=/var/run/virtiofsd/'
> +	    .'vm'.$vmid.'-fs'.$fsid.' -o source='.$path.' -o cache=always');

run_command like that is pretty much always wrong ;) please use

run_command(['usr/lib/kvm/virtiofsd', '--daemonize', ...])

and extract stuff into variables as needed if it gets too long/..

> +	POSIX::_exit(0);
> +    } elsif (!defined($pid)) {
> +        die "could not fork to start virtiofsd";
> +    }
> +}
> +
>  sub check_rng_source {
>      my ($source) = @_;
>  
> @@ -5497,6 +5598,18 @@ sub vm_start_nolock {
>      my ($cmd, $vollist, $spice_port) = config_to_command($storecfg, $vmid,
>  	$conf, $defaults, $forcemachine, $forcecpu, $params->{'pbs-backing'});
>  
> +    for (my $i = 0; $i < $MAX_SHAREDFILES; $i++) {
> +	my $sharedfilesstr = "sharedfiles$i";
> +
> +	next if !$conf->{$sharedfilesstr};
> +	my $sharedfiles = parse_sharedfiles($conf->{$sharedfilesstr});
> +	next if !$sharedfiles;
> +
> +	if ($sharedfiles && $sharedfiles->{type} eq 'virtio-fs' && !$conf->{numa}) {
> +	    start_virtiofs($vmid, $sharedfiles->{path}, $i);
> +	}
> +    }
> +
>      my $migration_ip;
>      my $get_migration_ip = sub {
>  	my ($nodename) = @_;
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>