Re: [pve-devel] [PATCH storage v4 02/12] storage: replace build-in stat with File::stat

[Alwin Antreich]

On Thu, Apr 23, 2020 at 05:52:29AM +0200, Dietmar Maurer wrote:
> > On April 22, 2020 6:00 PM Alwin Antreich  wrote:
> > 
> >  
> > On Wed, Apr 22, 2020 at 05:35:05PM +0200, Dietmar Maurer wrote:
> > > AFAIK this can have ugly side effects ...
> > Okay, I was not aware of any know side effects.
> > 
> > I took the File::stat, since we use it already in pve-cluster,
> > qemu-server, pve-common, ... . And a off-list discussion with Thomas and
> > Fabian G.
> > 
> > If there is a better solution, I am happy to work on it.
> 
> 
> # grep -r "use File::stat" /usr/share/perl5/PVE/
> /usr/share/perl5/PVE/QemuServer/Helpers.pm:use File::stat;
> /usr/share/perl5/PVE/Storage/ISCSIPlugin.pm:use File::stat;
> /usr/share/perl5/PVE/APIServer/AnyEvent.pm:use File::stat qw();
> /usr/share/perl5/PVE/AccessControl.pm:use File::stat;
> /usr/share/perl5/PVE/Cluster.pm:use File::stat qw();
> /usr/share/perl5/PVE/LXC/Setup/Base.pm:use File::stat;
> /usr/share/perl5/PVE/QemuServer.pm:use File::stat;
> /usr/share/perl5/PVE/INotify.pm:use File::stat;
> /usr/share/perl5/PVE/API2/APT.pm:use File::stat ();
> 
> So I would use:
> 
> use File::stat qw();
> 
> to avoid override the core stat() and lstat() functions.
Thank you. I will do that and add it to a v5.

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

Re: [pve-devel] [PATCH storage v4 02/12] storage: replace build-in stat with File::stat

[Dietmar Maurer]

> On April 22, 2020 6:00 PM Alwin Antreich  wrote:
> 
>  
> On Wed, Apr 22, 2020 at 05:35:05PM +0200, Dietmar Maurer wrote:
> > AFAIK this can have ugly side effects ...
> Okay, I was not aware of any know side effects.
> 
> I took the File::stat, since we use it already in pve-cluster,
> qemu-server, pve-common, ... . And a off-list discussion with Thomas and
> Fabian G.
> 
> If there is a better solution, I am happy to work on it.


# grep -r "use File::stat" /usr/share/perl5/PVE/
/usr/share/perl5/PVE/QemuServer/Helpers.pm:use File::stat;
/usr/share/perl5/PVE/Storage/ISCSIPlugin.pm:use File::stat;
/usr/share/perl5/PVE/APIServer/AnyEvent.pm:use File::stat qw();
/usr/share/perl5/PVE/AccessControl.pm:use File::stat;
/usr/share/perl5/PVE/Cluster.pm:use File::stat qw();
/usr/share/perl5/PVE/LXC/Setup/Base.pm:use File::stat;
/usr/share/perl5/PVE/QemuServer.pm:use File::stat;
/usr/share/perl5/PVE/INotify.pm:use File::stat;
/usr/share/perl5/PVE/API2/APT.pm:use File::stat ();

So I would use:

use File::stat qw();

to avoid override the core stat() and lstat() functions.

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

Re: [pve-devel] [PATCH storage v4 02/12] storage: replace build-in stat with File::stat

[Alwin Antreich]

On Wed, Apr 22, 2020 at 05:35:05PM +0200, Dietmar Maurer wrote:
> AFAIK this can have ugly side effects ...
Okay, I was not aware of any know side effects.

I took the File::stat, since we use it already in pve-cluster,
qemu-server, pve-common, ... . And a off-list discussion with Thomas and
Fabian G.

If there is a better solution, I am happy to work on it.

> 
> > On April 22, 2020 4:57 PM Alwin Antreich  wrote:
> > 
> >  
> > to minimize variable declarations. And allow to mock this method in
> > tests instead of the perl build-in stat.
> > 
> > Signed-off-by: Alwin Antreich 
> > ---
> >  PVE/Diskmanage.pm |  9 +
> >  PVE/Storage/Plugin.pm | 34 ++
> >  2 files changed, 15 insertions(+), 28 deletions(-)
> > 
> > diff --git a/PVE/Diskmanage.pm b/PVE/Diskmanage.pm
> > index 13e7cd8..cac944d 100644
> > --- a/PVE/Diskmanage.pm
> > +++ b/PVE/Diskmanage.pm
> > @@ -6,6 +6,7 @@ use PVE::ProcFSTools;
> >  use Data::Dumper;
> >  use Cwd qw(abs_path);
> >  use Fcntl ':mode';
> > +use File::stat;
> >  use JSON;
> >  
> >  use PVE::Tools qw(extract_param run_command file_get_contents 
> > file_read_firstline dir_glob_regex dir_glob_foreach trim);
> > @@ -673,11 +674,11 @@ sub get_disks {
> >  sub get_partnum {
> >  my ($part_path) = @_;
> >  
> > -my ($mode, $rdev) = (stat($part_path))[2,6];
> > +my $st = stat($part_path);
> >  
> > -next if !$mode || !S_ISBLK($mode) || !$rdev;
> > -my $major = PVE::Tools::dev_t_major($rdev);
> > -my $minor = PVE::Tools::dev_t_minor($rdev);
> > +next if !$st->mode || !S_ISBLK($st->mode) || !$st->rdev;
> > +my $major = PVE::Tools::dev_t_major($st->rdev);
> > +my $minor = PVE::Tools::dev_t_minor($st->rdev);
> >  my $partnum_path = "/sys/dev/block/$major:$minor/";
> >  
> >  my $partnum;
> > diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> > index 4489a77..d2dfad6 100644
> > --- a/PVE/Storage/Plugin.pm
> > +++ b/PVE/Storage/Plugin.pm
> > @@ -7,6 +7,7 @@ use Fcntl ':mode';
> >  use File::chdir;
> >  use File::Path;
> >  use File::Basename;
> > +use File::stat;
> >  use Time::Local qw(timelocal);
> >  
> >  use PVE::Tools qw(run_command);
> > @@ -718,12 +719,10 @@ sub free_image {
> >  sub file_size_info {
> >  my ($filename, $timeout) = @_;
> >  
> > -my @fs = stat($filename);
> > -my $mode = $fs[2];
> > -my $ctime = $fs[10];
> > +my $st = stat($filename);
> >  
> > -if (S_ISDIR($mode)) {
> > -   return wantarray ? (0, 'subvol', 0, undef, $ctime) : 1;
> > +if (S_ISDIR($st->mode)) {
> > +   return wantarray ? (0, 'subvol', 0, undef, $st->ctime) : 1;
> >  }
> >  
> >  my $json = '';
> > @@ -741,7 +740,7 @@ sub file_size_info {
> >  
> >  my ($size, $format, $used, $parent) = $info->@{qw(virtual-size format 
> > actual-size backing-filename)};
> >  
> > -return wantarray ? ($size, $format, $used, $parent, $ctime) : $size;
> > +return wantarray ? ($size, $format, $used, $parent, $st->ctime) : 
> > $size;
> >  }
> >  
> >  sub volume_size_info {
> > @@ -918,22 +917,9 @@ my $get_subdir_files = sub {
> >  
> >  foreach my $fn (<$path/*>) {
> >  
> > -   my ($dev,
> > -   $ino,
> > -   $mode,
> > -   $nlink,
> > -   $uid,
> > -   $gid,
> > -   $rdev,
> > -   $size,
> > -   $atime,
> > -   $mtime,
> > -   $ctime,
> > -   $blksize,
> > -   $blocks
> > -   ) = stat($fn);
> > -
> > -   next if S_ISDIR($mode);
> > +   my $st = stat($fn);
> > +
> > +   next if S_ISDIR($st->mode);
> >  
> > my $info;
> >  
> > @@ -972,8 +958,8 @@ my $get_subdir_files = sub {
> > };
> > }
> >  
> > -   $info->{size} = $size;
> > -   $info->{ctime} //= $ctime;
> > +   $info->{size} = $st->size;
> > +   $info->{ctime} //= $st->ctime;
> >  
> > push @$res, $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 mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH storage v4 02/12] storage: replace build-in stat with File::stat

[Dietmar Maurer]

AFAIK this can have ugly side effects ...

> On April 22, 2020 4:57 PM Alwin Antreich  wrote:
> 
>  
> to minimize variable declarations. And allow to mock this method in
> tests instead of the perl build-in stat.
> 
> Signed-off-by: Alwin Antreich 
> ---
>  PVE/Diskmanage.pm |  9 +
>  PVE/Storage/Plugin.pm | 34 ++
>  2 files changed, 15 insertions(+), 28 deletions(-)
> 
> diff --git a/PVE/Diskmanage.pm b/PVE/Diskmanage.pm
> index 13e7cd8..cac944d 100644
> --- a/PVE/Diskmanage.pm
> +++ b/PVE/Diskmanage.pm
> @@ -6,6 +6,7 @@ use PVE::ProcFSTools;
>  use Data::Dumper;
>  use Cwd qw(abs_path);
>  use Fcntl ':mode';
> +use File::stat;
>  use JSON;
>  
>  use PVE::Tools qw(extract_param run_command file_get_contents 
> file_read_firstline dir_glob_regex dir_glob_foreach trim);
> @@ -673,11 +674,11 @@ sub get_disks {
>  sub get_partnum {
>  my ($part_path) = @_;
>  
> -my ($mode, $rdev) = (stat($part_path))[2,6];
> +my $st = stat($part_path);
>  
> -next if !$mode || !S_ISBLK($mode) || !$rdev;
> -my $major = PVE::Tools::dev_t_major($rdev);
> -my $minor = PVE::Tools::dev_t_minor($rdev);
> +next if !$st->mode || !S_ISBLK($st->mode) || !$st->rdev;
> +my $major = PVE::Tools::dev_t_major($st->rdev);
> +my $minor = PVE::Tools::dev_t_minor($st->rdev);
>  my $partnum_path = "/sys/dev/block/$major:$minor/";
>  
>  my $partnum;
> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index 4489a77..d2dfad6 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -7,6 +7,7 @@ use Fcntl ':mode';
>  use File::chdir;
>  use File::Path;
>  use File::Basename;
> +use File::stat;
>  use Time::Local qw(timelocal);
>  
>  use PVE::Tools qw(run_command);
> @@ -718,12 +719,10 @@ sub free_image {
>  sub file_size_info {
>  my ($filename, $timeout) = @_;
>  
> -my @fs = stat($filename);
> -my $mode = $fs[2];
> -my $ctime = $fs[10];
> +my $st = stat($filename);
>  
> -if (S_ISDIR($mode)) {
> - return wantarray ? (0, 'subvol', 0, undef, $ctime) : 1;
> +if (S_ISDIR($st->mode)) {
> + return wantarray ? (0, 'subvol', 0, undef, $st->ctime) : 1;
>  }
>  
>  my $json = '';
> @@ -741,7 +740,7 @@ sub file_size_info {
>  
>  my ($size, $format, $used, $parent) = $info->@{qw(virtual-size format 
> actual-size backing-filename)};
>  
> -return wantarray ? ($size, $format, $used, $parent, $ctime) : $size;
> +return wantarray ? ($size, $format, $used, $parent, $st->ctime) : $size;
>  }
>  
>  sub volume_size_info {
> @@ -918,22 +917,9 @@ my $get_subdir_files = sub {
>  
>  foreach my $fn (<$path/*>) {
>  
> - my ($dev,
> - $ino,
> - $mode,
> - $nlink,
> - $uid,
> - $gid,
> - $rdev,
> - $size,
> - $atime,
> - $mtime,
> - $ctime,
> - $blksize,
> - $blocks
> - ) = stat($fn);
> -
> - next if S_ISDIR($mode);
> + my $st = stat($fn);
> +
> + next if S_ISDIR($st->mode);
>  
>   my $info;
>  
> @@ -972,8 +958,8 @@ my $get_subdir_files = sub {
>   };
>   }
>  
> - $info->{size} = $size;
> - $info->{ctime} //= $ctime;
> + $info->{size} = $st->size;
> + $info->{ctime} //= $st->ctime;
>  
>   push @$res, $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 mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH storage v4 02/12] storage: replace build-in stat with File::stat

[Alwin Antreich]

to minimize variable declarations. And allow to mock this method in
tests instead of the perl build-in stat.

Signed-off-by: Alwin Antreich 
---
 PVE/Diskmanage.pm |  9 +
 PVE/Storage/Plugin.pm | 34 ++
 2 files changed, 15 insertions(+), 28 deletions(-)

diff --git a/PVE/Diskmanage.pm b/PVE/Diskmanage.pm
index 13e7cd8..cac944d 100644
--- a/PVE/Diskmanage.pm
+++ b/PVE/Diskmanage.pm
@@ -6,6 +6,7 @@ use PVE::ProcFSTools;
 use Data::Dumper;
 use Cwd qw(abs_path);
 use Fcntl ':mode';
+use File::stat;
 use JSON;
 
 use PVE::Tools qw(extract_param run_command file_get_contents 
file_read_firstline dir_glob_regex dir_glob_foreach trim);
@@ -673,11 +674,11 @@ sub get_disks {
 sub get_partnum {
 my ($part_path) = @_;
 
-my ($mode, $rdev) = (stat($part_path))[2,6];
+my $st = stat($part_path);
 
-next if !$mode || !S_ISBLK($mode) || !$rdev;
-my $major = PVE::Tools::dev_t_major($rdev);
-my $minor = PVE::Tools::dev_t_minor($rdev);
+next if !$st->mode || !S_ISBLK($st->mode) || !$st->rdev;
+my $major = PVE::Tools::dev_t_major($st->rdev);
+my $minor = PVE::Tools::dev_t_minor($st->rdev);
 my $partnum_path = "/sys/dev/block/$major:$minor/";
 
 my $partnum;
diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
index 4489a77..d2dfad6 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -7,6 +7,7 @@ use Fcntl ':mode';
 use File::chdir;
 use File::Path;
 use File::Basename;
+use File::stat;
 use Time::Local qw(timelocal);
 
 use PVE::Tools qw(run_command);
@@ -718,12 +719,10 @@ sub free_image {
 sub file_size_info {
 my ($filename, $timeout) = @_;
 
-my @fs = stat($filename);
-my $mode = $fs[2];
-my $ctime = $fs[10];
+my $st = stat($filename);
 
-if (S_ISDIR($mode)) {
-   return wantarray ? (0, 'subvol', 0, undef, $ctime) : 1;
+if (S_ISDIR($st->mode)) {
+   return wantarray ? (0, 'subvol', 0, undef, $st->ctime) : 1;
 }
 
 my $json = '';
@@ -741,7 +740,7 @@ sub file_size_info {
 
 my ($size, $format, $used, $parent) = $info->@{qw(virtual-size format 
actual-size backing-filename)};
 
-return wantarray ? ($size, $format, $used, $parent, $ctime) : $size;
+return wantarray ? ($size, $format, $used, $parent, $st->ctime) : $size;
 }
 
 sub volume_size_info {
@@ -918,22 +917,9 @@ my $get_subdir_files = sub {
 
 foreach my $fn (<$path/*>) {
 
-   my ($dev,
-   $ino,
-   $mode,
-   $nlink,
-   $uid,
-   $gid,
-   $rdev,
-   $size,
-   $atime,
-   $mtime,
-   $ctime,
-   $blksize,
-   $blocks
-   ) = stat($fn);
-
-   next if S_ISDIR($mode);
+   my $st = stat($fn);
+
+   next if S_ISDIR($st->mode);
 
my $info;
 
@@ -972,8 +958,8 @@ my $get_subdir_files = sub {
};
}
 
-   $info->{size} = $size;
-   $info->{ctime} //= $ctime;
+   $info->{size} = $st->size;
+   $info->{ctime} //= $st->ctime;
 
push @$res, $info;
 }
-- 
2.20.1


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