[pve-devel] [PATCH container 2/6] clear reboot request in vm_start

2019-11-12 Thread Oguz Bektas 
Signed-off-by: Oguz Bektas 
---
 src/PVE/LXC.pm | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index c77ee01..091d34a 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -1932,6 +1932,9 @@ sub userns_command {
 sub vm_start {
 my ($vmid, $conf, $skiplock) = @_;
 
+eval { clear_reboot_request($vmid) };
+warn $@ if $@;
+
 # apply pending changes while starting
 if (scalar(keys %{$conf->{pending}})) {
my $storecfg = PVE::Storage::config();
-- 
2.20.1

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

[pve-devel] [PATCH widget-toolkit 5/6] add reboot for containers into task description table

2019-11-12 Thread Oguz Bektas 
Signed-off-by: Oguz Bektas 
---
 Utils.js | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Utils.js b/Utils.js
index e3dcfcd..3a8fa9a 100644
--- a/Utils.js
+++ b/Utils.js
@@ -494,6 +494,7 @@ Ext.define('Proxmox.Utils', { utilities: {
vzmount: ['CT', gettext('Mount') ],
vzumount: ['CT', gettext('Unmount') ],
vzshutdown: ['CT', gettext('Shutdown') ],
+   vzreboot: ['CT', gettext('Reboot') ],
vzsuspend: [ 'CT', gettext('Suspend') ],
vzresume: [ 'CT', gettext('Resume') ],
hamigrate: [ 'HA', gettext('Migrate') ],
-- 
2.20.1

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

[pve-devel] [PATCH container 4/6] pct: add 'pct reboot'

2019-11-12 Thread Oguz Bektas 
Signed-off-by: Oguz Bektas 
---
 src/PVE/CLI/pct.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/PVE/CLI/pct.pm b/src/PVE/CLI/pct.pm
index 3a32de4..98e2c6e 100755
--- a/src/PVE/CLI/pct.pm
+++ b/src/PVE/CLI/pct.pm
@@ -836,7 +836,8 @@ our $cmddef = {
 resume => [ 'PVE::API2::LXC::Status', 'vm_resume', ['vmid'], { node => 
$nodename }, $upid_exit],
 shutdown => [ 'PVE::API2::LXC::Status', 'vm_shutdown', ['vmid'], { node => 
$nodename }, $upid_exit],
 stop => [ 'PVE::API2::LXC::Status', 'vm_stop', ['vmid'], { node => 
$nodename }, $upid_exit],
-
+reboot => [ 'PVE::API2::LXC::Status', 'vm_reboot', ['vmid'], { node => 
$nodename }, $upid_exit],
+
 clone => [ "PVE::API2::LXC", 'clone_vm', ['vmid', 'newid'], { node => 
$nodename }, $upid_exit ],
 migrate => [ "PVE::API2::LXC", 'migrate_vm', ['vmid', 'target'], { node => 
$nodename }, $upid_exit],
 move_volume => [ "PVE::API2::LXC", 'move_volume', ['vmid', 'volume', 
'storage'], { node => $nodename }, $upid_exit ],
-- 
2.20.1

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

[pve-devel] [PATCH 0/6] implement CT reboot

2019-11-12 Thread Oguz Bektas 
this patch series implements the ability to reboot containers
(main reason being to apply pending changes).

most of the code is same/very similar with the qemu counterpart.

i made the choice of using separate reboot triggers instead of the ones already
used in prestart etc. hooks. the separate triggers in /run/lxc/$vmid.reboot make
it possible to differentiate between an API reboot request and a reboot
request from within the container. (also wanted to make it as similar to
qemu as possible).

since the helper code around reboot triggers is pretty much the same, we could 
move these to guest-common in the future.

pve-container:

Oguz Bektas (4):
  add reboot helpers to be used by containers
  clear reboot request in vm_start
  api: add reboot api call
  pct: add 'pct reboot'

 src/PVE/API2/LXC/Status.pm | 52 ++
 src/PVE/CLI/pct.pm |  3 ++-
 src/PVE/LXC.pm | 44 
 3 files changed, 98 insertions(+), 1 deletion(-)


proxmox-widget-toolkit:

Oguz Bektas (1):
  add reboot for containers into task description table

 Utils.js | 1 +
 1 file changed, 1 insertion(+)


pve-manager:

Oguz Bektas (1):
  add reboot button for containers

 www/manager6/lxc/Config.js | 35 +--
 1 file changed, 21 insertions(+), 14 deletions(-)




-- 
2.20.1

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

[pve-devel] [PATCH manager 6/6] add reboot button for containers

2019-11-12 Thread Oguz Bektas 
also use the opportunity to refactor the shutdown button code into the
menu.

Signed-off-by: Oguz Bektas 
---
 www/manager6/lxc/Config.js | 35 +--
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/www/manager6/lxc/Config.js b/www/manager6/lxc/Config.js
index 0f81c1da..5c760f13 100644
--- a/www/manager6/lxc/Config.js
+++ b/www/manager6/lxc/Config.js
@@ -52,18 +52,6 @@ Ext.define('PVE.lxc.Config', {
iconCls: 'fa fa-play'
});
 
-   var stopBtn = Ext.create('Ext.menu.Item',{
-   text: gettext('Stop'),
-   disabled: !caps.vms['VM.PowerMgmt'],
-   confirmMsg: Proxmox.Utils.format_task_description('vzstop', vmid),
-   tooltip: Ext.String.format(gettext('Stop {0} immediately'), 'CT'),
-   dangerous: true,
-   handler: function() {
-   vm_command("stop");
-   },
-   iconCls: 'fa fa-stop'
-   });
-
var shutdownBtn = Ext.create('PVE.button.Split', {
text: gettext('Shutdown'),
disabled: !caps.vms['VM.PowerMgmt'] || !running,
@@ -73,7 +61,27 @@ Ext.define('PVE.lxc.Config', {
vm_command('shutdown');
},
menu: {
-   items:[stopBtn]
+   items: [{
+   text: gettext('Stop'),
+   disabled: !caps.vms['VM.PowerMgmt'],
+   confirmMsg: Proxmox.Utils.format_task_description('vzstop', 
vmid),
+   tooltip: Ext.String.format(gettext('Stop {0} immediately'), 
'CT'),
+   dangerous: true,
+   handler: function() {
+   vm_command("stop");
+   },
+   iconCls: 'fa fa-stop'
+
+   },{
+   text: gettext('Reboot'),
+   disabled: !caps.vms['VM.PowerMgmt'],
+   confirmMsg: 
Proxmox.Utils.format_task_description('vzreboot', vmid),
+   tooltip: Ext.String.format(gettext('Reboot {0}'), 'CT'),
+   handler: function() {
+   vm_command("reboot");
+   },
+   iconCls: 'fa fa-refresh'
+   }]
},
iconCls: 'fa fa-power-off'
});
@@ -344,7 +352,6 @@ Ext.define('PVE.lxc.Config', {
 
startBtn.setDisabled(!caps.vms['VM.PowerMgmt'] || status === 
'running' || template);
shutdownBtn.setDisabled(!caps.vms['VM.PowerMgmt'] || status !== 
'running');
-   stopBtn.setDisabled(!caps.vms['VM.PowerMgmt'] || status === 
'stopped');
me.down('#removeBtn').setDisabled(!caps.vms['VM.Allocate'] || 
status !== 'stopped');
consoleBtn.setDisabled(template);
});
-- 
2.20.1

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

[pve-devel] [PATCH container 3/6] api: add reboot api call

2019-11-12 Thread Oguz Bektas 
pretty much the same code with the qemu counterpart, minus the qmp
stuff.

Signed-off-by: Oguz Bektas 
---
 src/PVE/API2/LXC/Status.pm | 52 ++
 1 file changed, 52 insertions(+)

diff --git a/src/PVE/API2/LXC/Status.pm b/src/PVE/API2/LXC/Status.pm
index 1b7a71d..406f1b0 100644
--- a/src/PVE/API2/LXC/Status.pm
+++ b/src/PVE/API2/LXC/Status.pm
@@ -65,6 +65,7 @@ __PACKAGE__->register_method({
{ subdir => 'start' },
{ subdir => 'stop' },
{ subdir => 'shutdown' },
+   { subdir => 'reboot' },
{ subdir => 'migrate' },
];
 
@@ -445,4 +446,55 @@ __PACKAGE__->register_method({
return $upid;
 }});
 
+__PACKAGE__->register_method({
+name => 'vm_reboot',
+path => 'reboot',
+method => 'POST',
+protected => 1,
+proxyto => 'node',
+description => "Reboot the container by shutting it down, and starting it 
again. Applies pending changes.",
+permissions => {
+   check => ['perm', '/vms/{vmid}', [ 'VM.PowerMgmt' ]],
+},
+parameters => {
+   additionalProperties => 0,
+   properties => {
+   node => get_standard_option('pve-node'),
+   vmid => get_standard_option('pve-vmid',
+   { completion => 
\::LXC::complete_ctid_running }),
+   timeout => {
+   description => "Wait maximal timeout seconds for the shutdown.",
+   type => 'integer',
+   minimum => 0,
+   optional => 1,
+   },
+   },
+},
+returns => {
+   type => 'string',
+},
+code => sub {
+   my ($param) = @_;
+
+   my $rpcenv = PVE::RPCEnvironment::get();
+   my $authuser = $rpcenv->get_user();
+
+   my $node = extract_param($param, 'node');
+   my $vmid = extract_param($param, 'vmid');
+
+   die "VM $vmid not running\n" if !PVE::LXC::check_running($vmid);
+
+   my $realcmd = sub {
+   my $upid = shift;
+
+   syslog('info', "requesting reboot of CT $vmid: $upid\n");
+   PVE::LXC::vm_reboot($vmid, $param->{timeout});
+   return;
+   };
+
+   return $rpcenv->fork_worker('vzreboot', $vmid, $authuser, $realcmd);
+}});
+
+
+
 1;
-- 
2.20.1

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

[pve-devel] [PATCH container 1/6] add reboot helpers to be used by containers

2019-11-12 Thread Oguz Bektas 
code for create_reboot_request and clear_reboot_request is from qemu, the only
difference is that we use /run/lxc/$vmid.reboot path instead of
/run/qemu-server.

there _is_ actually reboot triggers for lxc which are used by the
prestart hook and similar, however i think it's better if we can
differentiate between reboot requests from inside the container and from
API. (inadvertently this also allows to reboot container from inside without
applying pending changes)

since we don't have to clean up like in qemu, we can simply run vm_stop
and vm_start for a reboot operation.

Signed-off-by: Oguz Bektas 
---
 src/PVE/LXC.pm | 41 +
 1 file changed, 41 insertions(+)

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index cdf6d64..c77ee01 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -2013,6 +2013,47 @@ sub vm_stop {
 die "container did not stop\n";
 }
 
+sub create_reboot_request {
+my ($vmid) = @_;
+open(my $fh, '>', "/run/lxc/$vmid.reboot")
+   or die "failed to create reboot trigger file: $!\n";
+close($fh);
+}
+
+sub clear_reboot_request {
+my ($vmid) = @_;
+my $path = "/run/lxc/$vmid.reboot";
+my $res = 0;
+
+if (-e $path) {
+   $res = unlink($path);
+   die "could not remove reboot request for $vmid: $!\n" if !$res;
+}
+
+return $res;
+}
+
+sub vm_reboot {
+my ($vmid, $timeout, $skiplock) = @_;
+
+PVE::LXC::Config->lock_config($vmid, sub {
+   eval {
+   return if !check_running($vmid);
+
+   create_reboot_request($vmid);
+   vm_stop($vmid, 0, $timeout, 1); # kill if timeout exceeds
+
+   my $conf = PVE::LXC::Config->load_config($vmid);
+   vm_start($vmid, $conf);
+   };
+   if (my $err = $@) {
+   # avoid that the next normal shutdown will be confused for a reboot
+   clear_reboot_request($vmid);
+   die $err;
+   }
+});
+}
+
 sub run_unshared {
 my ($code) = @_;
 
-- 
2.20.1

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

[pve-devel] applied: [PATCH kernel] disable alsa snd pcspkr module

2019-11-12 Thread Thomas Lamprecht 
The PC speaker (beeper) can only be managed by one module, and there
are two which could do so. The very basic INPUT_PCSPKR, and the more
advanced SND_PCSP which allows it to be used as primitive ALSA
soundcard, which for Proxmox Server projects, and all modern
workstations is not much of use.

As they both were aliased to the "pcspkr" module name, and used the
same internal driver name (being a replacment of the other), one
would get the following error message when both are loaded:
"Error: Driver 'pcspkr' is already registered, aborting..."
in the kernel log. This happens as by default both are tried to get
loaded. We do not want the more complex ALSA one, so disable that.

Signed-off-by: Thomas Lamprecht 
---
 debian/rules | 1 +
 1 file changed, 1 insertion(+)

diff --git a/debian/rules b/debian/rules
index 24f027fe1f50..d344d4c1cc42 100755
--- a/debian/rules
+++ b/debian/rules
@@ -27,6 +27,7 @@ PVE_CONFIG_OPTS= \
 -m CONFIG_CEPH_FS \
 -m CONFIG_BLK_DEV_NBD \
 -m CONFIG_BLK_DEV_RBD \
+-d CONFIG_SND_PCSP \
 -m CONFIG_BCACHE \
 -m CONFIG_JFS_FS \
 -m CONFIG_HFS_FS \
-- 
2.20.1


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

Re: [pve-devel] [PATCH ct/common] mount point hotplugging & new mount api

2019-11-12 Thread Oguz Bektas 
On Tue, Nov 12, 2019 at 03:09:27PM +0100, Oguz Bektas wrote:
> hi,
> 
> built the latest git version of pve-common and pve-container with
> wolfgang's patches.
> 
> with running kernel: 5.0.21-4-pve
> and the latest pve-kernel-5.3
forgot to mention that it worked as expected with the newer kernel
> 
> found a small issue while testing.
> 
> when one has an older kernel and tries to hotplug a mountpoint
> 
> ---
> Parameter verification failed. (400)
> 
> mp1: unable to hotplug mp1: Can't use an undefined value as a symbol
> reference at /usr/share/perl5/PVE/LXC.pm line 1670. 
> ---
> 
> and around this line is:
> 
> ---
> 
> PVE::Tools::move_mount(
> fileno($mount_fd),
> "",
> _FDCWD,
> $mp->{mp},
> _MOUNT_F_EMPTY_PATH, # line 1670
> );
> });
> }
> 
> ---
> 
> so i'm guessing since that syscall doesn't exist in older kernels,
> we get an undefined value.
> 
> it would be better to handle this error with something more
> user-friendly and verbose.
> 
> i'm still testing and will write here if i notice something else.
> 
> ___
> 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

Re: [pve-devel] [PATCH ct/common] mount point hotplugging & new mount api

2019-11-12 Thread Oguz Bektas 
hi,

built the latest git version of pve-common and pve-container with
wolfgang's patches.

with running kernel: 5.0.21-4-pve
and the latest pve-kernel-5.3

found a small issue while testing.

when one has an older kernel and tries to hotplug a mountpoint

---
Parameter verification failed. (400)

mp1: unable to hotplug mp1: Can't use an undefined value as a symbol
reference at /usr/share/perl5/PVE/LXC.pm line 1670. 
---

and around this line is:

---

PVE::Tools::move_mount(
fileno($mount_fd),
"",
_FDCWD,
$mp->{mp},
_MOUNT_F_EMPTY_PATH, # line 1670
);
});
}

---

so i'm guessing since that syscall doesn't exist in older kernels,
we get an undefined value.

it would be better to handle this error with something more
user-friendly and verbose.

i'm still testing and will write here if i notice something else.

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

Re: [pve-devel] [PATCH common 1/1] SysFSTools: do not assume pci domain 0000

2019-11-12 Thread Dominik Csapak 

On 11/12/19 2:55 PM, Thomas Lamprecht wrote:

On 11/12/19 2:23 PM, Dominik Csapak wrote:

but prepend '' to ids where no domain is given, to keep the ability
to use the shorthand syntax (e.g. 00:01.0 instead of :00:01.0)

Signed-off-by: Dominik Csapak 
---
  src/PVE/SysFSTools.pm | 20 +---
  1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/src/PVE/SysFSTools.pm b/src/PVE/SysFSTools.pm
index 2da3a38..a8d9a7f 100644
--- a/src/PVE/SysFSTools.pm
+++ b/src/PVE/SysFSTools.pm
@@ -73,9 +73,9 @@ sub lspci {
  
  dir_glob_foreach("$pcisysfs/devices", $pciregex, sub {

my ($fullid, $domain, $bus, $slot, $function) = @_;
-   my $id = "$bus:$slot.$function";
+   my $id = "$domain:$bus:$slot.$function";
  
-	if (defined($filter) && !ref($filter) && $id !~ m/^\Q$filter\E/) {

+   if (defined($filter) && !ref($filter) && $id !~ 
m/^(:)?\Q$filter\E/) {
return; # filter ids early
}
  
@@ -279,13 +279,15 @@ sub pci_dev_group_bind_to_vfio {

  }
  die "Cannot find vfio-pci module!\n" if !-d $vfio_basedir;
  
+$pciid = ":$pciid" if $pciid !~ m/^[0-9a-f]{4}:/;

+
  # get IOMMU group devices
-opendir(my $D, "$pcisysfs/devices/:$pciid/iommu_group/devices/") || die 
"Cannot open iommu_group: $!\n";
-  my @devs = grep /^:/, readdir($D);
+opendir(my $D, "$pcisysfs/devices/$pciid/iommu_group/devices/") || die "Cannot 
open iommu_group: $!\n";
+  my @devs = grep /^[0-9a-f]{4}:/, readdir($D);
  closedir($D);
  
  foreach my $pciid (@devs) {

-   $pciid =~ m/^([:\.\da-f]+)$/ or die "PCI ID $pciid not valid!\n";
+   $pciid =~ m/^([:\.0-9a-f]+)$/ or die "PCI ID $pciid not valid!\n";
  
  # pci bridges, switches or root ports are not supported

  # they have a pci_bus subdirectory so skip them
@@ -301,7 +303,9 @@ sub pci_dev_group_bind_to_vfio {
  sub pci_create_mdev_device {
  my ($pciid, $uuid, $type) = @_;
  
-my $basedir = "$pcisysfs/devices/:$pciid";

+$pciid = ":$pciid" if $pciid !~ m/^[0-9a-f]{4}:/;
+
+my $basedir = "$pcisysfs/devices/$pciid";
  my $mdev_dir = "$basedir/mdev_supported_types";
  
  die "pci device '$pciid' does not support mediated devices \n"

@@ -336,7 +340,9 @@ sub pci_create_mdev_device {
  sub pci_cleanup_mdev_device {
  my ($pciid, $uuid) = @_;
  
-my $basedir = "$pcisysfs/devices/:$pciid/$uuid";

+$pciid = ":$pciid" if $pciid !~ m/^[0-9a-f]{4}:/;


maybe above could be moved in a helper?
could either return the "full" ID or a tuple like:
my ($full, $domain, $bus, $slot) = ...

not to sure though, and can also be OK without this for the
few call places..


i thought about this too, but we only use it 3 times, and it did
not seem worth it for a single line of code

and the only place where we use the domain/bus/slot split is in
the 'pci_device_info' sub...




+
+my $basedir = "$pcisysfs/devices/$pciid/$uuid";
  
  if (! -e $basedir) {

return 1; # no cleanup necessary if it does not exist




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

Re: [pve-devel] [PATCH common 1/1] SysFSTools: do not assume pci domain 0000

2019-11-12 Thread Thomas Lamprecht 
On 11/12/19 2:23 PM, Dominik Csapak wrote:
> but prepend '' to ids where no domain is given, to keep the ability
> to use the shorthand syntax (e.g. 00:01.0 instead of :00:01.0)
> 
> Signed-off-by: Dominik Csapak 
> ---
>  src/PVE/SysFSTools.pm | 20 +---
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/src/PVE/SysFSTools.pm b/src/PVE/SysFSTools.pm
> index 2da3a38..a8d9a7f 100644
> --- a/src/PVE/SysFSTools.pm
> +++ b/src/PVE/SysFSTools.pm
> @@ -73,9 +73,9 @@ sub lspci {
>  
>  dir_glob_foreach("$pcisysfs/devices", $pciregex, sub {
>   my ($fullid, $domain, $bus, $slot, $function) = @_;
> - my $id = "$bus:$slot.$function";
> + my $id = "$domain:$bus:$slot.$function";
>  
> - if (defined($filter) && !ref($filter) && $id !~ m/^\Q$filter\E/) {
> + if (defined($filter) && !ref($filter) && $id !~ 
> m/^(:)?\Q$filter\E/) {
>   return; # filter ids early
>   }
>  
> @@ -279,13 +279,15 @@ sub pci_dev_group_bind_to_vfio {
>  }
>  die "Cannot find vfio-pci module!\n" if !-d $vfio_basedir;
>  
> +$pciid = ":$pciid" if $pciid !~ m/^[0-9a-f]{4}:/;
> +
>  # get IOMMU group devices
> -opendir(my $D, "$pcisysfs/devices/:$pciid/iommu_group/devices/") || 
> die "Cannot open iommu_group: $!\n";
> -  my @devs = grep /^:/, readdir($D);
> +opendir(my $D, "$pcisysfs/devices/$pciid/iommu_group/devices/") || die 
> "Cannot open iommu_group: $!\n";
> +  my @devs = grep /^[0-9a-f]{4}:/, readdir($D);
>  closedir($D);
>  
>  foreach my $pciid (@devs) {
> - $pciid =~ m/^([:\.\da-f]+)$/ or die "PCI ID $pciid not valid!\n";
> + $pciid =~ m/^([:\.0-9a-f]+)$/ or die "PCI ID $pciid not valid!\n";
>  
>  # pci bridges, switches or root ports are not supported
>  # they have a pci_bus subdirectory so skip them
> @@ -301,7 +303,9 @@ sub pci_dev_group_bind_to_vfio {
>  sub pci_create_mdev_device {
>  my ($pciid, $uuid, $type) = @_;
>  
> -my $basedir = "$pcisysfs/devices/:$pciid";
> +$pciid = ":$pciid" if $pciid !~ m/^[0-9a-f]{4}:/;
> +
> +my $basedir = "$pcisysfs/devices/$pciid";
>  my $mdev_dir = "$basedir/mdev_supported_types";
>  
>  die "pci device '$pciid' does not support mediated devices \n"
> @@ -336,7 +340,9 @@ sub pci_create_mdev_device {
>  sub pci_cleanup_mdev_device {
>  my ($pciid, $uuid) = @_;
>  
> -my $basedir = "$pcisysfs/devices/:$pciid/$uuid";
> +$pciid = ":$pciid" if $pciid !~ m/^[0-9a-f]{4}:/;

maybe above could be moved in a helper?
could either return the "full" ID or a tuple like:
my ($full, $domain, $bus, $slot) = ...

not to sure though, and can also be OK without this for the
few call places..

> +
> +my $basedir = "$pcisysfs/devices/$pciid/$uuid";
>  
>  if (! -e $basedir) {
>   return 1; # no cleanup necessary if it does not exist
>

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

[pve-devel] [PATCH common/qemu-server/manager] fix #2436: do not hardcode pci domain

2019-11-12 Thread Dominik Csapak 
this series improves the pci passthrouh/selection code such that we
do not assume a pci domain of '' anymore. old configs still work,
and the 'default domain', , will not be written out to the config
(but shown in the gui)

we can ignore the changes in qemus commandline, since such vms can
neither be migrated nor snapshotted

dependencies:

new qemu-server needs pve-common
new pve-manager needs pve-common
new pve-common breaks old manager and qemu-server

the only issue occurs if one wants to edit a vm on a not upgraded
cluster member (the selected id does then not exist in the store...)
can we ignore this? if not, i would send a v2 where i try to detect
the format we get from the backend and set the value in the selector
accordingly (but i am not sure if its worth the effort, since partially
upgrading a cluster is not recommended)

pve-common:

Dominik Csapak (1):
  SysFSTools: do not assume pci domain 

 src/PVE/SysFSTools.pm | 20 +---
 1 file changed, 13 insertions(+), 7 deletions(-)

qemu-server:

Dominik Csapak (1):
  fix #2436: pci: do not hardcode pci domain to 

 PVE/QemuServer.pm | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

pve-manager:

Dominik Csapak (1):
  gui: pci passthrough: consider domain in PCISelector

 www/manager6/form/PCISelector.js |  2 +-
 www/manager6/qemu/PCIEdit.js | 11 +--
 2 files changed, 10 insertions(+), 3 deletions(-)

-- 
2.20.1


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

[pve-devel] [PATCH common 1/1] SysFSTools: do not assume pci domain 0000

2019-11-12 Thread Dominik Csapak 
but prepend '' to ids where no domain is given, to keep the ability
to use the shorthand syntax (e.g. 00:01.0 instead of :00:01.0)

Signed-off-by: Dominik Csapak 
---
 src/PVE/SysFSTools.pm | 20 +---
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/src/PVE/SysFSTools.pm b/src/PVE/SysFSTools.pm
index 2da3a38..a8d9a7f 100644
--- a/src/PVE/SysFSTools.pm
+++ b/src/PVE/SysFSTools.pm
@@ -73,9 +73,9 @@ sub lspci {
 
 dir_glob_foreach("$pcisysfs/devices", $pciregex, sub {
my ($fullid, $domain, $bus, $slot, $function) = @_;
-   my $id = "$bus:$slot.$function";
+   my $id = "$domain:$bus:$slot.$function";
 
-   if (defined($filter) && !ref($filter) && $id !~ m/^\Q$filter\E/) {
+   if (defined($filter) && !ref($filter) && $id !~ 
m/^(:)?\Q$filter\E/) {
return; # filter ids early
}
 
@@ -279,13 +279,15 @@ sub pci_dev_group_bind_to_vfio {
 }
 die "Cannot find vfio-pci module!\n" if !-d $vfio_basedir;
 
+$pciid = ":$pciid" if $pciid !~ m/^[0-9a-f]{4}:/;
+
 # get IOMMU group devices
-opendir(my $D, "$pcisysfs/devices/:$pciid/iommu_group/devices/") || 
die "Cannot open iommu_group: $!\n";
-  my @devs = grep /^:/, readdir($D);
+opendir(my $D, "$pcisysfs/devices/$pciid/iommu_group/devices/") || die 
"Cannot open iommu_group: $!\n";
+  my @devs = grep /^[0-9a-f]{4}:/, readdir($D);
 closedir($D);
 
 foreach my $pciid (@devs) {
-   $pciid =~ m/^([:\.\da-f]+)$/ or die "PCI ID $pciid not valid!\n";
+   $pciid =~ m/^([:\.0-9a-f]+)$/ or die "PCI ID $pciid not valid!\n";
 
 # pci bridges, switches or root ports are not supported
 # they have a pci_bus subdirectory so skip them
@@ -301,7 +303,9 @@ sub pci_dev_group_bind_to_vfio {
 sub pci_create_mdev_device {
 my ($pciid, $uuid, $type) = @_;
 
-my $basedir = "$pcisysfs/devices/:$pciid";
+$pciid = ":$pciid" if $pciid !~ m/^[0-9a-f]{4}:/;
+
+my $basedir = "$pcisysfs/devices/$pciid";
 my $mdev_dir = "$basedir/mdev_supported_types";
 
 die "pci device '$pciid' does not support mediated devices \n"
@@ -336,7 +340,9 @@ sub pci_create_mdev_device {
 sub pci_cleanup_mdev_device {
 my ($pciid, $uuid) = @_;
 
-my $basedir = "$pcisysfs/devices/:$pciid/$uuid";
+$pciid = ":$pciid" if $pciid !~ m/^[0-9a-f]{4}:/;
+
+my $basedir = "$pcisysfs/devices/$pciid/$uuid";
 
 if (! -e $basedir) {
return 1; # no cleanup necessary if it does not exist
-- 
2.20.1


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

[pve-devel] [PATCH qemu-server 1/1] fix #2436: pci: do not hardcode pci domain to 0000

2019-11-12 Thread Dominik Csapak 
relax the regex for hostpci to allow different pci domains than 

Signed-off-by: Dominik Csapak 
---
 PVE/QemuServer.pm | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 1890448..709dcdb 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -1337,7 +1337,7 @@ my $usbdesc = {
 };
 PVE::JSONSchema::register_standard_option("pve-qm-usb", $usbdesc);
 
-my $PCIRE = qr/[a-f0-9]{2}:[a-f0-9]{2}(?:\.[a-f0-9])?/;
+my $PCIRE = qr/([a-f0-9]{4}:)?[a-f0-9]{2}:[a-f0-9]{2}(?:\.[a-f0-9])?/;
 my $hostpci_fmt = {
 host => {
default_key => 1,
@@ -3709,7 +3709,7 @@ sub config_to_command {
if ($d->{mdev} && scalar(@$pcidevices) == 1) {
my $pci_id = $pcidevices->[0]->{id};
my $uuid = PVE::SysFSTools::generate_mdev_uuid($vmid, $i);
-   $sysfspath = "/sys/bus/pci/devices/:$pci_id/$uuid";
+   $sysfspath = "/sys/bus/pci/devices/$pci_id/$uuid";
} elsif ($d->{mdev}) {
warn "ignoring mediated device '$id' with multifunction device\n";
}
@@ -5394,7 +5394,7 @@ sub vm_start {
  foreach my $pcidevice (@$pcidevices) {
my $pciid = $pcidevice->{id};
 
-   my $info = PVE::SysFSTools::pci_device_info(":$pciid");
+   my $info = PVE::SysFSTools::pci_device_info("$pciid");
die "IOMMU not present\n" if 
!PVE::SysFSTools::check_iommu_support();
die "no pci device info for device '$pciid'\n" if !$info;
 
-- 
2.20.1


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

[pve-devel] [PATCH manager 1/1] gui: pci passthrough: consider domain in PCISelector

2019-11-12 Thread Dominik Csapak 
but remove the default domain '' before sending to the backend,
and add it if no domain is given in the config

Signed-off-by: Dominik Csapak 
---
 www/manager6/form/PCISelector.js |  2 +-
 www/manager6/qemu/PCIEdit.js | 11 +--
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/www/manager6/form/PCISelector.js b/www/manager6/form/PCISelector.js
index 1061ef4d..c6847b67 100644
--- a/www/manager6/form/PCISelector.js
+++ b/www/manager6/form/PCISelector.js
@@ -27,7 +27,7 @@ Ext.define('PVE.form.PCISelector', {
{
header: 'ID',
dataIndex: 'id',
-   width: 80
+   width: 100
},
{
header: gettext('IOMMU Group'),
diff --git a/www/manager6/qemu/PCIEdit.js b/www/manager6/qemu/PCIEdit.js
index 6ff13808..1853d241 100644
--- a/www/manager6/qemu/PCIEdit.js
+++ b/www/manager6/qemu/PCIEdit.js
@@ -10,7 +10,10 @@ Ext.define('PVE.qemu.PCIInputPanel', {
var hostpci = me.vmconfig[me.confid] || '';
 
var values = PVE.Parser.parsePropertyString(hostpci, 'host');
-   if (values.host && values.host.length < 6) { // 00:00 format not 00:00.0
+   if (!values.host.match(/^[0-9a-f]{4}:/i)) { // add optional domain
+   values.host = ":" + values.host;
+   }
+   if (values.host && values.host.length < 11) { // :00:00 format not 
:00:00.0
values.host += ".0";
values.multifunction = true;
}
@@ -43,9 +46,13 @@ Ext.define('PVE.qemu.PCIInputPanel', {
}
}
}
+   // remove optional '' domain
+   if (values.host.substring(0,5) === ':') {
+   values.host = values.host.substring(5);
+   }
if (values.multifunction) {
// modify host to skip the '.X'
-   values.host = values.host.substring(0,5);
+   values.host = values.host.substring(0, values.host.indexOf('.'));
delete values.multifunction;
}
 
-- 
2.20.1


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

[pve-devel] applied: [PATCH common] fix PVE::Tools::df for big mounts

2019-11-12 Thread Thomas Lamprecht 
On 11/12/19 1:56 PM, Dominik Csapak wrote:
> if the size/avail of a mount is bigger than a certain amount,
> json_encode writes the number in scientific format, which did not
> fit inside our \d+ regex
> 
> this resulted in 'undef' values for the result hash and subsequently
> led to errors and warnings
> 
> extend it to also catch scientific formatted numbers
> 
> Signed-off-by: Dominik Csapak 
> ---
>  src/PVE/Tools.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

applied, changed the commit message a bit, e.g., linked the forum thread
and noted that perl itself can use such format as is. Thanks!

> diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
> index 02c2886..89de4ec 100644
> --- a/src/PVE/Tools.pm
> +++ b/src/PVE/Tools.pm
> @@ -1008,7 +1008,7 @@ sub df {
>  warn $@ if $@;
>  
>  # untaint the values
> -my ($blocks, $used, $bavail) = map { defined($_) ? (/^(\d+)$/) : 0 }
> +my ($blocks, $used, $bavail) = map { defined($_) ? (/^([\d\.e\-+]+)$/) : 
> 0 } # can be in scientific notation

followed up with moving above comment into the untaint one,
avoiding very long lines, if not really necessary.

>   $res->@{qw(blocks used bavail)};
>  
>  return {
> 


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

[pve-devel] [PATCH pve-firewall 2/2] add synflood protection

2019-11-12 Thread Alexandre Derumier 
Currently, a virtio-net + vhost-net can handle between 200-300 kpps for each vm 
(with 1core/queue=1).
That mean than a vm can easily overloaded with a simple synflood (hping3 
--flood -p 80 -S targetip).
Also the conntrack of the host can be saturated easily.

This patch introduce a new option, enable rate limiting of syn/s by src ip 
(protection_synflood:1).

rate limit can be set with : protection_synflood_rate  (default 200 syn/s)
with an extra burst: protection_synflood_rate (default 1000).

It's also possible to reduce conntrack syn timeout: 
nf_conntrack_tcp_timeout_syn_recv (default 60).

with default values, a src ip can take around (60 * 200 = 12000 conntrack 
entries).

The iptables rules are done in raw table, before reaching the conntrack.

This protection works fine for non-spoofed src ip.
For spoofed src ip, the only way could be to implement SYNPROXY,
but this only works for routed/nat setup. (The host need to be able to reply
with the src ip the vm)

Some good information about synflood protections
https://2014.rmll.info/slides/356/day_1-1400-Jesper_Brouer-DDoS_protection_using_Netfilter_iptables.pdf
---
 src/PVE/Firewall.pm | 58 +++--
 1 file changed, 56 insertions(+), 2 deletions(-)

diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index 8f4ff1a..db16e0f 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -1273,6 +1273,14 @@ our $host_option_properties = {
default => 432000,
minimum => 7875,
 },
+nf_conntrack_tcp_timeout_syn_recv => {
+   description => "Conntrack syn recv timeout.",
+   type => 'integer',
+   optional => 1,
+   default => 60,
+   minimum => 30,
+   maximum => 60,
+},
 ndp => {
description => "Enable NDP (Neighbor Discovery Protocol).",
type => 'boolean',
@@ -1285,6 +1293,24 @@ our $host_option_properties = {
default => 0,
optional => 1,
 },
+protection_synflood => {
+   description => "Enable synflood protection",
+   type => 'boolean',
+   default => 0,
+   optional => 1,
+},
+protection_synflood_rate => {
+   description => "Synflood protection rate syn/sec by ip src.",
+   type => 'integer',
+   optional => 1,
+   default => 200,
+},
+protection_synflood_burst => {
+   description => "Synflood protection rate burst by ip src.",
+   type => 'integer',
+   optional => 1,
+   default => 1000,
+},
 log_nf_conntrack => {
description => "Enable logging of conntrack information.",
type => 'boolean',
@@ -2752,13 +2778,13 @@ sub parse_hostfw_option {
 
 my $loglevels = "emerg|alert|crit|err|warning|notice|info|debug|nolog";
 
-if ($line =~ 
m/^(enable|nosmurfs|tcpflags|ndp|log_nf_conntrack|nf_conntrack_allow_invalid):\s*(0|1)\s*$/i)
 {
+if ($line =~ 
m/^(enable|nosmurfs|tcpflags|ndp|log_nf_conntrack|nf_conntrack_allow_invalid|protection_synflood):\s*(0|1)\s*$/i)
 {
$opt = lc($1);
$value = int($2);
 } elsif ($line =~ 
m/^(log_level_in|log_level_out|tcp_flags_log_level|smurf_log_level):\s*(($loglevels)\s*)?$/i)
 {
$opt = lc($1);
$value = $2 ? lc($3) : '';
-} elsif ($line =~ 
m/^(nf_conntrack_max|nf_conntrack_tcp_timeout_established):\s*(\d+)\s*$/i) {
+} elsif ($line =~ 
m/^(nf_conntrack_max|nf_conntrack_tcp_timeout_established|nf_conntrack_tcp_timeout_syn_recv|protection_synflood_rate|protection_synflood_burst|protection_limit):\s*(\d+)\s*$/i)
 {
$opt = lc($1);
$value = int($2);
 } else {
@@ -3572,6 +3598,22 @@ sub compile_iptables_raw {
 
 my $ruleset = {};
 
+my $hostfw_options = $hostfw_conf->{options} || {};
+my $protection_synflood = $hostfw_options->{protection_synflood} || 0;
+
+if($protection_synflood) {
+
+   my $protection_synflood_rate = 
$hostfw_options->{protection_synflood_rate} ? 
$hostfw_options->{protection_synflood_rate} : 200;
+   my $protection_synflood_burst = 
$hostfw_options->{protection_synflood_burst} ? 
$hostfw_options->{protection_synflood_burst} : 1000;
+   my $protection_synflood_limit = 
$hostfw_options->{protection_synflood_limit} ? 
$hostfw_options->{protection_synflood_limit} : 3000;
+   my $protection_synflood_expire = 
$hostfw_options->{nf_conntrack_tcp_timeout_syn_recv} ? 
$hostfw_options->{nf_conntrack_tcp_timeout_syn_recv} : 60;
+   $protection_synflood_expire = $protection_synflood_expire * 1000;
+   my $protection_synflood_mask = $ipversion == 4 ? 32 : 64;
+
+   ruleset_create_chain($ruleset, "PVEFW-PREROUTING");
+   ruleset_addrule($ruleset, "PVEFW-PREROUTING", "-p tcp -m tcp 
--tcp-flags FIN,SYN,RST,ACK SYN -m hashlimit --hashlimit-above 
$protection_synflood_rate/sec --hashlimit-burst $protection_synflood_burst 
--hashlimit-mode srcip --hashlimit-name syn --hashlimit-htable-size 2097152 
--hashlimit-srcmask $protection_synflood_mask --hashlimit-htable-expire

[pve-devel] [PATCH pve-firewall 1/2] iptables : add raw table support

2019-11-12 Thread Alexandre Derumier 
---
 src/PVE/Firewall.pm | 122 +---
 src/PVE/Service/pve_firewall.pm |  27 ---
 test/fwtester.pl|  10 +--
 3 files changed, 119 insertions(+), 40 deletions(-)

diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index 97e5384..8f4ff1a 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -1757,15 +1757,17 @@ sub enable_bridge_firewall {
 }
 
 sub iptables_restore_cmdlist {
-my ($cmdlist) = @_;
+my ($cmdlist, $table) = @_;
 
-run_command(['iptables-restore', '-n'], input => $cmdlist, errmsg => 
"iptables_restore_cmdlist");
+$table = 'filter' if !$table;
+run_command(['iptables-restore', '-T', $table, '-n'], input => $cmdlist, 
errmsg => "iptables_restore_cmdlist");
 }
 
 sub ip6tables_restore_cmdlist {
-my ($cmdlist) = @_;
+my ($cmdlist, $table) = @_;
 
-run_command(['ip6tables-restore', '-n'], input => $cmdlist, errmsg => 
"iptables_restore_cmdlist");
+$table = 'filter' if !$table;
+run_command(['ip6tables-restore', '-T', $table, '-n'], input => $cmdlist, 
errmsg => "iptables_restore_cmdlist");
 }
 
 sub ipset_restore_cmdlist {
@@ -1781,9 +1783,10 @@ sub ebtables_restore_cmdlist {
 }
 
 sub iptables_get_chains {
-my ($iptablescmd) = @_;
+my ($iptablescmd, $t) = @_;
 
 $iptablescmd = "iptables" if !$iptablescmd;
+$t = 'filter' if !$t;
 
 my $res = {};
 
@@ -1818,7 +1821,7 @@ sub iptables_get_chains {
return;
}
 
-   return if $table ne 'filter';
+   return if $table ne $t;
 
if ($line =~ m/^:(\S+)\s/) {
my $chain = $1;
@@ -1828,7 +1831,7 @@ sub iptables_get_chains {
my ($chain, $sig) = ($1, $2);
return if !&$is_pvefw_chain($chain);
$res->{$chain} = $sig;
-   } elsif ($line =~ m/^-A\s+(INPUT|OUTPUT|FORWARD)\s+-j\s+PVEFW-\1$/) {
+   } elsif ($line =~ 
m/^-A\s+(INPUT|OUTPUT|FORWARD|PREROUTING)\s+-j\s+PVEFW-\1$/) {
$hooks->{$1} = 1;
} else {
# simply ignore the rest
@@ -3552,14 +3555,26 @@ sub compile {
 
 push @{$cluster_conf->{ipset}->{management}}, { cidr => $localnet };
 
-my $ruleset = compile_iptables_filter($cluster_conf, $hostfw_conf, 
$vmfw_configs, $vmdata, $corosync_conf, 4);
-my $rulesetv6 = compile_iptables_filter($cluster_conf, $hostfw_conf, 
$vmfw_configs, $vmdata, $corosync_conf, 6);
+my $ruleset = {};
+my $rulesetv6 = {};
+$ruleset->{filter} = compile_iptables_filter($cluster_conf, $hostfw_conf, 
$vmfw_configs, $vmdata, $corosync_conf, 4);
+$ruleset->{raw} = compile_iptables_raw($cluster_conf, $hostfw_conf, 
$vmfw_configs, $vmdata, $corosync_conf, 4);
+$rulesetv6->{filter} = compile_iptables_filter($cluster_conf, 
$hostfw_conf, $vmfw_configs, $vmdata, $corosync_conf, 6);
+$rulesetv6->{raw} = compile_iptables_raw($cluster_conf, $hostfw_conf, 
$vmfw_configs, $vmdata, $corosync_conf, 6);
 my $ebtables_ruleset = compile_ebtables_filter($cluster_conf, 
$hostfw_conf, $vmfw_configs, $vmdata);
 my $ipset_ruleset = compile_ipsets($cluster_conf, $vmfw_configs, $vmdata);
 
 return ($ruleset, $ipset_ruleset, $rulesetv6, $ebtables_ruleset);
 }
 
+sub compile_iptables_raw {
+my ($cluster_conf, $hostfw_conf, $vmfw_configs, $vmdata, $corosync_conf, 
$ipversion) = @_;
+
+my $ruleset = {};
+
+return $ruleset;
+}
+
 sub compile_iptables_filter {
 my ($cluster_conf, $hostfw_conf, $vmfw_configs, $vmdata, $corosync_conf, 
$ipversion) = @_;
 
@@ -3958,11 +3973,13 @@ sub print_sig_rule {
 }
 
 sub get_ruleset_cmdlist {
-my ($ruleset, $iptablescmd) = @_;
+my ($ruleset, $iptablescmd, $table) = @_;
 
-my $cmdlist = "*filter\n"; # we pass this to iptables-restore;
+$table = 'filter' if !$table;
 
-my ($active_chains, $hooks) = iptables_get_chains($iptablescmd);
+my $cmdlist = "*$table\n"; # we pass this to iptables-restore;
+
+my ($active_chains, $hooks) = iptables_get_chains($iptablescmd, $table);
 my $statushash = get_ruleset_status($ruleset, $active_chains, 
\_chain_digest);
 
 # create missing chains first
@@ -3974,7 +3991,7 @@ sub get_ruleset_cmdlist {
$cmdlist .= ":$chain - [0:0]\n";
 }
 
-foreach my $h (qw(INPUT OUTPUT FORWARD)) {
+foreach my $h (qw(INPUT OUTPUT FORWARD PREROUTING)) {
my $chain = "PVEFW-$h";
if ($ruleset->{$chain} && !$hooks->{$h}) {
$cmdlist .= "-A $h -j $chain\n";
@@ -4009,10 +4026,11 @@ sub get_ruleset_cmdlist {
next if $chain eq 'PVEFW-INPUT';
next if $chain eq 'PVEFW-OUTPUT';
next if $chain eq 'PVEFW-FORWARD';
+   next if $chain eq 'PVEFW-PREROUTING';
$cmdlist .= "-X $chain\n";
 }
 
-my $changes = $cmdlist ne "*filter\n" ? 1 : 0;
+my $changes = $cmdlist ne "*$table\n" ? 1 : 0;
 
 $cmdlist .= "COMMIT\n";
 
@@ -4125,9 +4143,11 @@ sub apply_ruleset {
 my ($ipset_create_cmdlist, $ipset_delete_cmdlist, $ipset_changes) =

[pve-devel] [PATCH pve-firewall 0/2] Fix #2450: synflood protection

2019-11-12 Thread Alexandre Derumier 
Currently, a virtio-net + vhost-net can handle between 200-300 kpps for each vm 
(with 1core/queue=1).
That mean than a vm can easily overloaded with a simple synflood (hping3 
--flood -p 80 -S targetip).
Also the conntrack of the host can be saturated easily.

This patch introduce a new option, enable rate limiting of syn/s by src ip 
(protection_synflood:1).

rate limit can be set with : protection_synflood_rate  (default 200 syn/s)
with an extra burst: protection_synflood_rate (default 1000).

It's also possible to reduce conntrack syn timeout: 
nf_conntrack_tcp_timeout_syn_recv (default 60).

with default values, a src ip can take around (60 * 200 = 12000 conntrack 
entries).

The iptables rules are done in raw table, before reaching the conntrack.

This protection works fine for non-spoofed src ip.
For spoofed src ip, the only way could be to implement SYNPROXY,
but this only works for routed/nat setup. (The host need to be able to reply
with the src ip the vm) and need 
https://bugzilla.proxmox.com/show_bug.cgi?id=2451

Some good information about synflood protections
https://2014.rmll.info/slides/356/day_1-1400-Jesper_Brouer-DDoS_protection_using_Netfilter_iptables.pdf


Alexandre Derumier (2):
  iptables : add raw table support
  add synflood protection

 src/PVE/Firewall.pm | 180 +++-
 src/PVE/Service/pve_firewall.pm |  27 +++--
 test/fwtester.pl|  10 +-
 3 files changed, 175 insertions(+), 42 deletions(-)

-- 
2.20.1

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

[pve-devel] [PATCH common] fix PVE::Tools::df for big mounts

2019-11-12 Thread Dominik Csapak 
if the size/avail of a mount is bigger than a certain amount,
json_encode writes the number in scientific format, which did not
fit inside our \d+ regex

this resulted in 'undef' values for the result hash and subsequently
led to errors and warnings

extend it to also catch scientific formatted numbers

Signed-off-by: Dominik Csapak 
---
 src/PVE/Tools.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
index 02c2886..89de4ec 100644
--- a/src/PVE/Tools.pm
+++ b/src/PVE/Tools.pm
@@ -1008,7 +1008,7 @@ sub df {
 warn $@ if $@;
 
 # untaint the values
-my ($blocks, $used, $bavail) = map { defined($_) ? (/^(\d+)$/) : 0 }
+my ($blocks, $used, $bavail) = map { defined($_) ? (/^([\d\.e\-+]+)$/) : 0 
} # can be in scientific notation
$res->@{qw(blocks used bavail)};
 
 return {
-- 
2.20.1


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

Re: [pve-devel] [PATCH storage] fix #2085: Handle non-default mount point in path() by introducing new mountpoint property

2019-11-12 Thread Thomas Lamprecht 
On 11/12/19 12:18 PM, Fabian Ebner wrote:
> 
> I don't see a clean way to do the automatic adding of the mount point 
> property (doing it in path() is possible, but we need to assume that our 
> caller doesn't hold the lock on storage.cfg). Maybe it's better to just do 
> the warning in activate_storage and leave it to the user to configure the 
> mount point properly? It feels a bit weird for the plugin code to modify the 
> config; is there an instance where it does?

You could just to this once at bootup, or initial mount.


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

[pve-devel] applied: [PATCH docu] Fix #2459: qm: Make info about core limit clear

2019-11-12 Thread Thomas Lamprecht 
On 11/12/19 10:08 AM, Dominic Jäger wrote:
> 'assigning' could also mean that creating a VM with more cores than physically
> available is impossible. However, this is not the case. Using 'starting'
> instead is more precise and still easy to understand.
> 
> Signed-off-by: Dominic Jäger 
> ---
>  qm.adoc | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 

Applied, thanks!


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

Re: [pve-devel] [PATCH storage] fix #2085: Handle non-default mount point in path() by introducing new mountpoint property

2019-11-12 Thread Fabian Grünbichler 
On November 12, 2019 12:18 pm, Fabian Ebner wrote:
> On 11/7/19 12:59 PM, Fabian Grünbichler wrote:
>> On November 7, 2019 12:52 pm, Fabian Ebner wrote:
>>> On 11/7/19 9:34 AM, Fabian Grünbichler wrote:
 On November 6, 2019 1:46 pm, Fabian Ebner wrote:
> A new mountpoint property is added to the schema for ZFSPool storages.
> When needed for the first time, the current mount point is determined and
> written to the storage config.
>
> Signed-off-by: Fabian Ebner 
> ---
>PVE/Storage/ZFSPoolPlugin.pm | 25 +++--
>1 file changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/PVE/Storage/ZFSPoolPlugin.pm b/PVE/Storage/ZFSPoolPlugin.pm
> index 16fb0d6..44c8123 100644
> --- a/PVE/Storage/ZFSPoolPlugin.pm
> +++ b/PVE/Storage/ZFSPoolPlugin.pm
> @@ -32,6 +32,10 @@ sub properties {
>   description => "use sparse volumes",
>   type => 'boolean',
>   },
> + mountpoint => {
> + description => "mount point",
> + type => 'string', format => 'pve-storage-path',
> + },
>};
>}
>
> @@ -44,6 +48,7 @@ sub options {
>   disable => { optional => 1 },
>   content => { optional => 1 },
>   bwlimit => { optional => 1 },
> + mountpoint => { optional => 1 },
>};
>}
>
> @@ -148,11 +153,27 @@ sub path {
>my ($vtype, $name, $vmid) = $class->parse_volname($volname);
>
>my $path = '';
> +my $mountpoint = $scfg->{mountpoint};
> +
> +# automatically write mountpoint to storage config if it is not 
> present
> +if (!$mountpoint) {
> + $mountpoint = $class->zfs_request($scfg, undef, 'get', '-o', 'value',
> +   'mountpoint', '-H', $scfg->{pool});
> + chomp($mountpoint);
> +
> + eval {
> + PVE::Storage::lock_storage_config(sub {
> + my $cfg = PVE::Storage::config();
> + $cfg->{ids}->{$storeid}->{mountpoint} = $mountpoint;
> + PVE::Storage::write_config($cfg);
> + }, "update storage config failed");
> + };
> + warn $@ if $@;
> +}
>
>if ($vtype eq "images") {
>   if ($name =~ m/^subvol-/ || $name =~ m/^basevol-/) {
> - # fixme: we currently assume standard mount point?!
> - $path = "/$scfg->{pool}/$name";
> + $path = "$mountpoint/$name";
>   } else {
>   $path = "/dev/zvol/$scfg->{pool}/$name";
>   }

 it would be great if this "get mountpoint property of dataset" (or better:
 even "get arbitrary properties of dataset") helper could be factored out.
 we already have two calls to "zfs get -Hp '...' $dataset" that could
 re-use such a helper.

 then we could add a single check in activate_storage (maybe just for the
 branch where we actually just imported the pool?) to verify that the
 stored mountpoint value is correct, and if not update it (or warn about
 the problem). that check could have a very low timeout, and errors could
 be ignored since it is just a double-check.

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

>>>
>>> I noticed that the $scfg parameter in zfs_request is never used. Should
>>> I clean that up? Otherwise I'd need to include the $scfg parameter in
>>> the helper function as well, where it also isn't needed.
>> 
>> it's used by ZFS over iSCSI, where the plugin is derived from
>> ZFSPoolPlugin, so it's not safe to remove ;)
>> 
>> ___
>> pve-devel mailing list
>> pve-devel@pve.proxmox.com
>> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>> 
> 
> 
> I see, thanks. I'll remember to check whether the method is virtual next 
> time.
> 
> There are two problems with doing the mount point configuration in 
> activate_storage.
> First, we might get called by 'create' (API for config triggered by 
> 'pvesm add') which holds a lock on storage.cfg at that time, but we 
> might also get called by something else without such a lock.
> Second, 'create' will write its own version of the config after calling 
> activate_storage anyways.
> 
> It also doesn't help to only do the mount point configuration inside the 
> branch where we just imported, since the pool/dataset might be imported 
> already when we do 'pvesm add' and the second problem is still there.
> 
> Now there is plugin->on_add_hook used by 'create' which would also be a 
> good place to configure the mount point. We could activate the storage 
> there, check for the mount point and then hijack the scfg that is passed 
> along to it (also not nice), so that it is written by 'create' later. In 
>

Re: [pve-devel] [PATCH storage] fix #2085: Handle non-default mount point in path() by introducing new mountpoint property

2019-11-12 Thread Fabian Ebner 

On 11/7/19 12:59 PM, Fabian Grünbichler wrote:

On November 7, 2019 12:52 pm, Fabian Ebner wrote:

On 11/7/19 9:34 AM, Fabian Grünbichler wrote:

On November 6, 2019 1:46 pm, Fabian Ebner wrote:

A new mountpoint property is added to the schema for ZFSPool storages.
When needed for the first time, the current mount point is determined and
written to the storage config.

Signed-off-by: Fabian Ebner 
---
   PVE/Storage/ZFSPoolPlugin.pm | 25 +++--
   1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/PVE/Storage/ZFSPoolPlugin.pm b/PVE/Storage/ZFSPoolPlugin.pm
index 16fb0d6..44c8123 100644
--- a/PVE/Storage/ZFSPoolPlugin.pm
+++ b/PVE/Storage/ZFSPoolPlugin.pm
@@ -32,6 +32,10 @@ sub properties {
description => "use sparse volumes",
type => 'boolean',
},
+   mountpoint => {
+   description => "mount point",
+   type => 'string', format => 'pve-storage-path',
+   },
   };
   }
   
@@ -44,6 +48,7 @@ sub options {

disable => { optional => 1 },
content => { optional => 1 },
bwlimit => { optional => 1 },
+   mountpoint => { optional => 1 },
   };
   }
   
@@ -148,11 +153,27 @@ sub path {

   my ($vtype, $name, $vmid) = $class->parse_volname($volname);
   
   my $path = '';

+my $mountpoint = $scfg->{mountpoint};
+
+# automatically write mountpoint to storage config if it is not present
+if (!$mountpoint) {
+   $mountpoint = $class->zfs_request($scfg, undef, 'get', '-o', 'value',
+ 'mountpoint', '-H', $scfg->{pool});
+   chomp($mountpoint);
+
+   eval {
+   PVE::Storage::lock_storage_config(sub {
+   my $cfg = PVE::Storage::config();
+   $cfg->{ids}->{$storeid}->{mountpoint} = $mountpoint;
+   PVE::Storage::write_config($cfg);
+   }, "update storage config failed");
+   };
+   warn $@ if $@;
+}
   
   if ($vtype eq "images") {

if ($name =~ m/^subvol-/ || $name =~ m/^basevol-/) {
-   # fixme: we currently assume standard mount point?!
-   $path = "/$scfg->{pool}/$name";
+   $path = "$mountpoint/$name";
} else {
$path = "/dev/zvol/$scfg->{pool}/$name";
}


it would be great if this "get mountpoint property of dataset" (or better:
even "get arbitrary properties of dataset") helper could be factored out.
we already have two calls to "zfs get -Hp '...' $dataset" that could
re-use such a helper.

then we could add a single check in activate_storage (maybe just for the
branch where we actually just imported the pool?) to verify that the
stored mountpoint value is correct, and if not update it (or warn about
the problem). that check could have a very low timeout, and errors could
be ignored since it is just a double-check.

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



I noticed that the $scfg parameter in zfs_request is never used. Should
I clean that up? Otherwise I'd need to include the $scfg parameter in
the helper function as well, where it also isn't needed.


it's used by ZFS over iSCSI, where the plugin is derived from
ZFSPoolPlugin, so it's not safe to remove ;)

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




I see, thanks. I'll remember to check whether the method is virtual next 
time.


There are two problems with doing the mount point configuration in 
activate_storage.
First, we might get called by 'create' (API for config triggered by 
'pvesm add') which holds a lock on storage.cfg at that time, but we 
might also get called by something else without such a lock.
Second, 'create' will write its own version of the config after calling 
activate_storage anyways.


It also doesn't help to only do the mount point configuration inside the 
branch where we just imported, since the pool/dataset might be imported 
already when we do 'pvesm add' and the second problem is still there.


Now there is plugin->on_add_hook used by 'create' which would also be a 
good place to configure the mount point. We could activate the storage 
there, check for the mount point and then hijack the scfg that is passed 
along to it (also not nice), so that it is written by 'create' later. In 
activate_storage we could still check whether configured and effective 
mount points match and warn if they don't.


I don't see a clean way to do the automatic adding of the mount point 
property (doing it in path() is possible, but we need to assume that our 
caller doesn't hold the lock on storage.cfg). Maybe it's better to just 
do the warning in activate_storage and leave it to the user to configure 
the mount point properly? It feels a bit weird for the plugin code to 
modify the config; is there an

Re: [pve-devel] [PATCH 17/23] allow ticket in auth header as fallback

2019-11-12 Thread Thomas Lamprecht 
On 11/12/19 11:17 AM, Fabian Grünbichler wrote:
> On November 12, 2019 11:05 am, Thomas Lamprecht wrote:
>> On 11/12/19 10:46 AM, Fabian Grünbichler wrote:
>>> On October 17, 2019 5:33 pm, Thomas Lamprecht wrote:
 On 10/17/19 3:14 PM, Fabian Grünbichler wrote:
> @@ -1232,7 +1232,10 @@ sub unshift_read_header {
>   } elsif ($path =~ m/^\Q$base_uri\E/) {
>   my $token = $r->header('CSRFPreventionToken');
>   my $cookie = $r->header('Cookie');
> - my $ticket = 
> PVE::APIServer::Formatter::extract_auth_cookie($cookie, 
> $self->{cookie_name});
> + my $auth_header = $r->header('Authorization');
> + my $ticket = 
> PVE::APIServer::Formatter::extract_auth_value($cookie, 
> $self->{cookie_name});
> + $ticket = 
> PVE::APIServer::Formatter::extract_auth_value($auth_header, 
> $self->{cookie_name})
> + if !$ticket;

 can we do this a bit more separate like:

 if (!$ticket && (my $auth_header = $r->header('Authorization')) {
 $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, 
 $self->{cookie_name});
 }
>>>
>>> this would then (with the next patch) become:
>>>
>>> my $api_token;
>>> if (!$ticket && (my $auth_header = $r->header('Authorization')) {
>>> $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, 
>>> $self->{cookie_name});
>>>
>>> if (!$ticket) {
>>> $api_token = 
>>> PVE::APIServer::Formatter::extract_auth_value($auth_header, 
>>> $self->{apitoken_name});
>>> }
>>> }
>>>
>> the inner (apitoken) "if" would be nicer to move a layer out below the other 
>> one.>>
> 
> it needs $auth_header though, which would then also move (back out 
> again):
> 
> my $auth_header = $r->header('Authorization');
> if (!$ticket) {
> $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, 
> $self->{cookie_name});
> }
> my $api_token;
> if (!$ticket) {
> $api_token = PVE::APIServer::Formatter::extract_auth_value($auth_header, 
> $self->{apitoken_name});
> }
> 
> which is basically the original version, modulo separate declaration of 
> $api_token:
> 
> my $auth_header = $r->header('Authorization');
> 
> $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, 
> $self->{cookie_name})
> if !$ticket;
> 
> my $api_token;
> $api_token = PVE::APIServer::Formatter::extract_auth_value($auth_header, 
> $self->{apitoken_name})
> if !$ticket;
> 

Above with line spacing and comments would already be a big
improvement. I feel that the real-nice-thing is still missing though,
but no better idea myself ^^


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

[pve-devel] applied: [PATCH manager] ui: vm opts: add hint for spice foldersharing

2019-11-12 Thread Thomas Lamprecht 
On 11/6/19 1:17 PM, Aaron Lauterer wrote:
> Spice foldersharing needs the webdavd daemon installed inside the guest.
> This patch adds a hint to remind the user to install it in the VM.
> 
> Signed-off-by: Aaron Lauterer 
> ---
>  www/manager6/form/SpiceEnhancementSelector.js | 13 +
>  1 file changed, 13 insertions(+)
> 


applied, thanks!

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

Re: [pve-devel] [PATCH 17/23] allow ticket in auth header as fallback

2019-11-12 Thread Fabian Grünbichler 
On November 12, 2019 11:05 am, Thomas Lamprecht wrote:
> On 11/12/19 10:46 AM, Fabian Grünbichler wrote:
>> On October 17, 2019 5:33 pm, Thomas Lamprecht wrote:
>>> On 10/17/19 3:14 PM, Fabian Grünbichler wrote:
 @@ -1232,7 +1232,10 @@ sub unshift_read_header {
} elsif ($path =~ m/^\Q$base_uri\E/) {
my $token = $r->header('CSRFPreventionToken');
my $cookie = $r->header('Cookie');
 -  my $ticket = 
 PVE::APIServer::Formatter::extract_auth_cookie($cookie, 
 $self->{cookie_name});
 +  my $auth_header = $r->header('Authorization');
 +  my $ticket = 
 PVE::APIServer::Formatter::extract_auth_value($cookie, 
 $self->{cookie_name});
 +  $ticket = 
 PVE::APIServer::Formatter::extract_auth_value($auth_header, 
 $self->{cookie_name})
 +  if !$ticket;
>>>
>>> can we do this a bit more separate like:
>>>
>>> if (!$ticket && (my $auth_header = $r->header('Authorization')) {
>>> $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, 
>>> $self->{cookie_name});
>>> }
>> 
>> this would then (with the next patch) become:
>> 
>> my $api_token;
>> if (!$ticket && (my $auth_header = $r->header('Authorization')) {
>> $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, 
>> $self->{cookie_name});
>> 
>> if (!$ticket) {
>> $api_token = 
>> PVE::APIServer::Formatter::extract_auth_value($auth_header, 
>> $self->{apitoken_name});
>> }
>> }
>> 
> the inner (apitoken) "if" would be nicer to move a layer out below the other 
> one.>>

it needs $auth_header though, which would then also move (back out 
again):

my $auth_header = $r->header('Authorization');
if (!$ticket) {
$ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, 
$self->{cookie_name});
}
my $api_token;
if (!$ticket) {
$api_token = PVE::APIServer::Formatter::extract_auth_value($auth_header, 
$self->{apitoken_name});
}

which is basically the original version, modulo separate declaration of 
$api_token:

my $auth_header = $r->header('Authorization');

$ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, 
$self->{cookie_name})
if !$ticket;

my $api_token;
$api_token = PVE::APIServer::Formatter::extract_auth_value($auth_header, 
$self->{apitoken_name})
if !$ticket;

>>> or if you prefer:
>>>
>>> if (!$ticket) {
>>> my $auth_header = $r->header('Authorization');
>>> $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, 
>>> $self->{cookie_name});
>>> }
>> 
>> my $api_token;
>>  if (!$ticket) {
>>  my $auth_header = $r->header('Authorization');
>>  $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, 
>> $self->{cookie_name});
>> 
>>  if (!$ticket) {
>> $api_token = 
>> PVE::APIServer::Formatter::extract_auth_value($auth_header, 
>> $self->{apitoken_name});
>>  }
>>  }
>> 
>>>
>>> makes it slightly cleaner/easier to read, IMO
>> 
>> which makes it harder to parse IMHO, but if you still prefer it I will 
>> rewrite it with your suggestion for v2? we could of course also add 
>> comments, like so:
>> 
>> # check actual cookie
> s/check/prefer/
> 
>> my $ticket = PVE::APIServer::Formatter::extract_auth_value($cookie, 
>> $self->{cookie_name});
>> 
>> # fallback to cookie in 'Authorization' header
>> $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, 
>> $self->{cookie_name})
>> if !$ticket;
>> 
>> # fallback to API token
>> my $api_token = PVE::APIServer::Formatter::extract_auth_value($auth_header, 
>> $self->{apitoken_name})
>> if !$ticket;
> NAK, do *not* declare variables guarded with a postfix if, that gets
> bad results.[0][1]

fair enough (I always forgot about this) - it's a separate issue from 
the question of whether to nest or not nest the if's though ;)

> 
> [0]: https://pve.proxmox.com/pipermail/pve-devel/2018-June/032238.html
> [1]: https://perldoc.perl.org/perlsyn.html#Statement-Modifiers
> 
> 

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

Re: [pve-devel] [PATCH 17/23] allow ticket in auth header as fallback

2019-11-12 Thread Thomas Lamprecht 
On 11/12/19 10:46 AM, Fabian Grünbichler wrote:
> On October 17, 2019 5:33 pm, Thomas Lamprecht wrote:
>> On 10/17/19 3:14 PM, Fabian Grünbichler wrote:
>>> @@ -1232,7 +1232,10 @@ sub unshift_read_header {
>>> } elsif ($path =~ m/^\Q$base_uri\E/) {
>>> my $token = $r->header('CSRFPreventionToken');
>>> my $cookie = $r->header('Cookie');
>>> -   my $ticket = 
>>> PVE::APIServer::Formatter::extract_auth_cookie($cookie, 
>>> $self->{cookie_name});
>>> +   my $auth_header = $r->header('Authorization');
>>> +   my $ticket = 
>>> PVE::APIServer::Formatter::extract_auth_value($cookie, 
>>> $self->{cookie_name});
>>> +   $ticket = 
>>> PVE::APIServer::Formatter::extract_auth_value($auth_header, 
>>> $self->{cookie_name})
>>> +   if !$ticket;
>>
>> can we do this a bit more separate like:
>>
>> if (!$ticket && (my $auth_header = $r->header('Authorization')) {
>> $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, 
>> $self->{cookie_name});
>> }
> 
> this would then (with the next patch) become:
> 
> my $api_token;
> if (!$ticket && (my $auth_header = $r->header('Authorization')) {
> $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, 
> $self->{cookie_name});
> 
> if (!$ticket) {
> $api_token = 
> PVE::APIServer::Formatter::extract_auth_value($auth_header, 
> $self->{apitoken_name});
> }
> }
> 
the inner (apitoken) "if" would be nicer to move a layer out below the other 
one.>>
>> or if you prefer:
>>
>> if (!$ticket) {
>> my $auth_header = $r->header('Authorization');
>> $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, 
>> $self->{cookie_name});
>> }
> 
> my $api_token;
>  if (!$ticket) {
>  my $auth_header = $r->header('Authorization');
>  $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, 
> $self->{cookie_name});
> 
>  if (!$ticket) {
> $api_token = 
> PVE::APIServer::Formatter::extract_auth_value($auth_header, 
> $self->{apitoken_name});
>  }
>  }
> 
>>
>> makes it slightly cleaner/easier to read, IMO
> 
> which makes it harder to parse IMHO, but if you still prefer it I will 
> rewrite it with your suggestion for v2? we could of course also add 
> comments, like so:
> 
> # check actual cookie
s/check/prefer/

> my $ticket = PVE::APIServer::Formatter::extract_auth_value($cookie, 
> $self->{cookie_name});
> 
> # fallback to cookie in 'Authorization' header
> $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, 
> $self->{cookie_name})
> if !$ticket;
> 
> # fallback to API token
> my $api_token = PVE::APIServer::Formatter::extract_auth_value($auth_header, 
> $self->{apitoken_name})
> if !$ticket;
NAK, do *not* declare variables guarded with a postfix if, that gets
bad results.[0][1]

[0]: https://pve.proxmox.com/pipermail/pve-devel/2018-June/032238.html
[1]: https://perldoc.perl.org/perlsyn.html#Statement-Modifiers


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

[pve-devel] [PATCH qemu-server] Use crm-command stop to allow shutdown with timeout and hard stop for HA

2019-11-12 Thread Fabian Ebner 
The minimum value for timeout in vm_shutdown is changed from 0 to 1, since a
value of 0 would trigger a hard stop for HA managed VMs. Like this the API
description stays valid for all cases.

Signed-off-by: Fabian Ebner 
---

In vm_shutdown we'd like to pass along the timeout parameter to the HA stack,
but the value 0 triggers a special behavior there, namely a hard stop.
So either the description needs to be changed to mention this behavior or
we just don't allow a timeout of 0 in vm_shutdown. Since a shutdown with
timeout 0 doesn't really make sense anyways, I opted for the latter.
It's the same situation for containers.

 PVE/API2/Qemu.pm | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index c31dd1d..16a7a04 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -2111,7 +2111,7 @@ __PACKAGE__->register_method({
 
print "Requesting HA stop for VM $vmid\n";
 
-   my $cmd = ['ha-manager', 'set',  "vm:$vmid", '--state', 
'stopped'];
+   my $cmd = ['ha-manager', 'crm-command', 'stop',  "vm:$vmid", 
'0'];
PVE::Tools::run_command($cmd);
return;
};
@@ -2204,7 +2204,7 @@ __PACKAGE__->register_method({
timeout => {
description => "Wait maximal timeout seconds.",
type => 'integer',
-   minimum => 0,
+   minimum => 1,
optional => 1,
},
forceStop => {
@@ -2267,12 +2267,13 @@ __PACKAGE__->register_method({
 
if (PVE::HA::Config::vm_is_ha_managed($vmid) && $rpcenv->{type} ne 
'ha') {
 
+   my $timeout = $param->{timeout} // 60;
my $hacmd = sub {
my $upid = shift;
 
print "Requesting HA stop for VM $vmid\n";
 
-   my $cmd = ['ha-manager', 'set', "vm:$vmid", '--state', 
'stopped'];
+   my $cmd = ['ha-manager', 'crm-command', 'stop', "vm:$vmid", 
"$timeout"];
PVE::Tools::run_command($cmd);
return;
};
-- 
2.20.1


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

[pve-devel] [PATCH container] Use crm-command stop to allow shutdown with timeout and hard stop for HA

2019-11-12 Thread Fabian Ebner 
The minimum value for timeout in vm_shutdown is changed from 0 to 1, since a
value of 0 would trigger a hard stop for HA managed containers. Like this the
API description stays valid for all cases.

Signed-off-by: Fabian Ebner 
---
 src/PVE/API2/LXC/Status.pm | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/PVE/API2/LXC/Status.pm b/src/PVE/API2/LXC/Status.pm
index 1b7a71d..20b376a 100644
--- a/src/PVE/API2/LXC/Status.pm
+++ b/src/PVE/API2/LXC/Status.pm
@@ -243,7 +243,7 @@ __PACKAGE__->register_method({
 
print "Requesting HA stop for CT $vmid\n";
 
-   my $cmd = ['ha-manager', 'set',  "ct:$vmid", '--state', 
'stopped'];
+   my $cmd = ['ha-manager', 'crm-command', 'stop', "ct:$vmid", 
'0'];
PVE::Tools::run_command($cmd);
};
 
@@ -291,7 +291,7 @@ __PACKAGE__->register_method({
timeout => {
description => "Wait maximal timeout seconds.",
type => 'integer',
-   minimum => 0,
+   minimum => 1,
optional => 1,
default => 60,
},
@@ -325,7 +325,7 @@ __PACKAGE__->register_method({
 
print "Requesting HA stop for CT $vmid\n";
 
-   my $cmd = ['ha-manager', 'set',  "ct:$vmid", '--state', 
'stopped'];
+   my $cmd = ['ha-manager', 'crm-command', 'stop',  "ct:$vmid", 
"$timeout"];
PVE::Tools::run_command($cmd);
};
 
-- 
2.20.1


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

Re: [pve-devel] [PATCH 17/23] allow ticket in auth header as fallback

2019-11-12 Thread Fabian Grünbichler 
On October 17, 2019 5:33 pm, Thomas Lamprecht wrote:
> On 10/17/19 3:14 PM, Fabian Grünbichler wrote:
>> based on idea & RFC by Tim Marx, incorporating feedback by Thomas
>> Lamprecht. this will be extended to support API tokens in the
>> Authorization header as well, so make it generic.
>> 
>> Signed-off-by: Fabian Grünbichler 
>> ---
>> 
>> Notes:
>> semi-independent, could also leave extract_auth_cookie as alias/wrapper 
>> to
>> avoid a change in PMG. but since we need to change other method 
>> signatures
>> anyway for the token part, we could change this as well.
>> 
>> as-is, needs a versioned breaks/depends on pve-manager and some part of 
>> PMG?
>> 
>>  PVE/APIServer/AnyEvent.pm  |  5 -
>>  PVE/APIServer/Formatter.pm | 12 ++--
>>  2 files changed, 10 insertions(+), 7 deletions(-)
>> 
>> diff --git a/PVE/APIServer/AnyEvent.pm b/PVE/APIServer/AnyEvent.pm
>> index 9aba27d..c0b90ab 100644
>> --- a/PVE/APIServer/AnyEvent.pm
>> +++ b/PVE/APIServer/AnyEvent.pm
>> @@ -1232,7 +1232,10 @@ sub unshift_read_header {
>>  } elsif ($path =~ m/^\Q$base_uri\E/) {
>>  my $token = $r->header('CSRFPreventionToken');
>>  my $cookie = $r->header('Cookie');
>> -my $ticket = 
>> PVE::APIServer::Formatter::extract_auth_cookie($cookie, 
>> $self->{cookie_name});
>> +my $auth_header = $r->header('Authorization');
>> +my $ticket = 
>> PVE::APIServer::Formatter::extract_auth_value($cookie, $self->{cookie_name});
>> +$ticket = 
>> PVE::APIServer::Formatter::extract_auth_value($auth_header, 
>> $self->{cookie_name})
>> +if !$ticket;
> 
> can we do this a bit more separate like:
> 
> if (!$ticket && (my $auth_header = $r->header('Authorization')) {
> $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, 
> $self->{cookie_name});
> }

this would then (with the next patch) become:

my $api_token;
if (!$ticket && (my $auth_header = $r->header('Authorization')) {
$ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, 
$self->{cookie_name});

if (!$ticket) {
$api_token = 
PVE::APIServer::Formatter::extract_auth_value($auth_header, 
$self->{apitoken_name});
}
}

> 
> or if you prefer:
> 
> if (!$ticket) {
> my $auth_header = $r->header('Authorization');
> $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, 
> $self->{cookie_name});
> }

my $api_token;
 if (!$ticket) {
 my $auth_header = $r->header('Authorization');
 $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, 
$self->{cookie_name});

 if (!$ticket) {
$api_token = 
PVE::APIServer::Formatter::extract_auth_value($auth_header, 
$self->{apitoken_name});
 }
 }

> 
> makes it slightly cleaner/easier to read, IMO

which makes it harder to parse IMHO, but if you still prefer it I will 
rewrite it with your suggestion for v2? we could of course also add 
comments, like so:

# check actual cookie
my $ticket = PVE::APIServer::Formatter::extract_auth_value($cookie, 
$self->{cookie_name});

# fallback to cookie in 'Authorization' header
$ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, 
$self->{cookie_name})
if !$ticket;

# fallback to API token
my $api_token = PVE::APIServer::Formatter::extract_auth_value($auth_header, 
$self->{apitoken_name})
if !$ticket;

> 
>>  
>>  my ($rel_uri, $format) = &$split_abs_uri($path, 
>> $self->{base_uri});
>>  if (!$format) {
>> diff --git a/PVE/APIServer/Formatter.pm b/PVE/APIServer/Formatter.pm
>> index 0c459bd..def1932 100644
>> --- a/PVE/APIServer/Formatter.pm
>> +++ b/PVE/APIServer/Formatter.pm
>> @@ -75,16 +75,16 @@ sub get_login_formatter {
>>  
>>  # some helper functions
>>  
>> -sub extract_auth_cookie {
>> -my ($cookie, $cookie_name) = @_;
>> +sub extract_auth_value {
>> +my ($header, $key) = @_;
>>  
>> -return undef if !$cookie;
>> +return undef if !$header;
>>  
>> -my $ticket = ($cookie =~ /(?:^|\s)\Q$cookie_name\E=([^;]*)/)[0];
>> +my $value = ($header =~ /(?:^|\s)\Q$key\E(?:=| )([^;]*)/)[0];
>>  
>> -$ticket = uri_unescape($ticket) if $ticket;
>> +$value = uri_unescape($value) if $value;
>>  
>> -return $ticket;
>> +return $value;
>>  }
>>  
>>  sub create_auth_cookie {
>> 
> 
> 
> 

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

[pve-devel] [PATCH docu] Fix #2459: qm: Make info about core limit clear

2019-11-12 Thread Dominic Jäger 
'assigning' could also mean that creating a VM with more cores than physically
available is impossible. However, this is not the case. Using 'starting'
instead is more precise and still easy to understand.

Signed-off-by: Dominic Jäger 
---
 qm.adoc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qm.adoc b/qm.adoc
index 100d315..b13f0f4 100644
--- a/qm.adoc
+++ b/qm.adoc
@@ -283,8 +283,8 @@ is greater than the number of cores on the server (e.g., 4 
VMs with each 4
 cores on a machine with only 8 cores). In that case the host system will
 balance the Qemu execution threads between your server cores, just like if you
 were running a standard multithreaded application. However, {pve} will prevent
-you from assigning more virtual CPU cores than physically available, as this 
will
-only bring the performance down due to the cost of context switches.
+you from starting VMs with more virtual CPU cores than physically available, as
+this will only bring the performance down due to the cost of context switches.
 
 [[qm_cpu_resource_limits]]
 Resource Limits
-- 
2.20.1

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