Re: [pve-devel] [PATCH librados2-perl] Split method pve_rados_connect

[Alwin Antreich]

On Tue, Apr 03, 2018 at 10:25:53AM +0200, Thomas Lamprecht wrote:
>
> Am 03/30/2018 um 12:25 PM schrieb Alwin Antreich:
> > To be able to connect through librados2 without a config file, the
> > method pve_rados_connect is split up into pve_rados_connect and
> > pve_rados_conf_read_file.
> >
> > Signed-off-by: Alwin Antreich 
> > ---
> >   PVE/RADOS.pm |  9 -
> >   RADOS.xs | 26 +-
> >   2 files changed, 29 insertions(+), 6 deletions(-)
> >
> > diff --git a/PVE/RADOS.pm b/PVE/RADOS.pm
> > index aa6a102..ad1c2db 100644
> > --- a/PVE/RADOS.pm
> > +++ b/PVE/RADOS.pm
> > @@ -1,6 +1,6 @@
> >   package PVE::RADOS;
> > -use 5.014002;
> > +use 5.014002; # FIXME: update version??
> >   use strict;
> >   use warnings;
> >   use Carp;
> > @@ -13,6 +13,7 @@ use PVE::RPCEnvironment;
> >   require Exporter;
> >   my $rados_default_timeout = 5;
> > +my $ceph_default_conf = '/etc/ceph/ceph.conf';
> >   our @ISA = qw(Exporter);
> > @@ -164,6 +165,12 @@ sub new {
> > $conn = pve_rados_create() ||
> > die "unable to create RADOS object\n";
> > +   my $ceph_conf = delete $params{ceph_conf} || $ceph_default_conf;
> > +
> > +   if (-e $ceph_conf) {
> > +   pve_rados_conf_read_file($conn, $ceph_conf);
> > +   }
> > +
> > pve_rados_conf_set($conn, 'client_mount_timeout', $timeout);
> > foreach my $k (keys %params) {
> > diff --git a/RADOS.xs b/RADOS.xs
> > index a9f6bc3..ad3cf96 100644
> > --- a/RADOS.xs
> > +++ b/RADOS.xs
> > @@ -47,19 +47,35 @@ CODE:
>
> This whole hunk does not apply here...
> A quick look gave me one whitespace problem (see below), but that alone did
> not fix it for me...
> Are you sure you sent all commits between this and origin/master ?
>
> git log origin/master..
I will add a white space cleanup before my v2 of this patch, as it seems
not to apply without the cleanup.

>
>
> >   }
> >   void
> > -pve_rados_connect(cluster)
> > +pve_rados_conf_read_file(cluster, path)
> >   rados_t cluster
> > -PROTOTYPE: $
> > +SV *path
> > +PROTOTYPE: $$
> >   CODE:
> >   {
> > -DPRINTF("pve_rados_connect\n");
> > +char *p = NULL;
> > -int res = rados_conf_read_file(cluster, NULL);
> > +if (SvOK(path)) {
> > +   p = SvPV_nolen(path);
> > +}
> > +
> > +DPRINTF("pve_rados_conf_read_file %s\n", p);
> > +
> > +int res = rados_conf_read_file(cluster, p);
> >   if (res < 0) {
> >   die("rados_conf_read_file failed - %s\n", strerror(-res));
> >   }
> > +}
> > +
> > +void
> > +pve_rados_connect(cluster)
> > +rados_t cluster
> > +PROTOTYPE: $
> > +CODE:
> > +{
> > +DPRINTF("pve_rados_connect\n");
>
> The empty line above contains a trailing whitespace in origin/master, which
> your patch does not contain.
see above

>
> > -res = rados_connect(cluster);
> > +int res = rados_connect(cluster);
> >   if (res < 0) {
> >   die("rados_connect failed - %s\n", strerror(-res));
> >   }
>

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

Re: [pve-devel] [PATCH librados2-perl] Split method pve_rados_connect

[Alwin Antreich]

On Tue, Apr 03, 2018 at 02:13:18PM +0200, Dietmar Maurer wrote:
> comments inline
>
> > On March 30, 2018 at 12:25 PM Alwin Antreich  wrote:
> >
> >
> > To be able to connect through librados2 without a config file, the
> > method pve_rados_connect is split up into pve_rados_connect and
> > pve_rados_conf_read_file.
> >
> > Signed-off-by: Alwin Antreich 
> > ---
> >  PVE/RADOS.pm |  9 -
> >  RADOS.xs | 26 +-
> >  2 files changed, 29 insertions(+), 6 deletions(-)
> >
> > diff --git a/PVE/RADOS.pm b/PVE/RADOS.pm
> > index aa6a102..ad1c2db 100644
> > --- a/PVE/RADOS.pm
> > +++ b/PVE/RADOS.pm
> > @@ -1,6 +1,6 @@
> >  package PVE::RADOS;
> >
> > -use 5.014002;
> > +use 5.014002; # FIXME: update version??
>
> why this FIXME?
On debian stretch there is a newer perl version v5.24.1, thought that we
might want to change it.

>
> >  use strict;
> >  use warnings;
> >  use Carp;
> > @@ -13,6 +13,7 @@ use PVE::RPCEnvironment;
> >  require Exporter;
> >
> >  my $rados_default_timeout = 5;
> > +my $ceph_default_conf = '/etc/ceph/ceph.conf';
> >
> >
> >  our @ISA = qw(Exporter);
> > @@ -164,6 +165,12 @@ sub new {
> > $conn = pve_rados_create() ||
> > die "unable to create RADOS object\n";
> >
> > +   my $ceph_conf = delete $params{ceph_conf} || $ceph_default_conf;
> > +
> > +   if (-e $ceph_conf) {
> > +   pve_rados_conf_read_file($conn, $ceph_conf);
> > +   }
> > +
>
> What if $params{ceph_conf} is set, but file does not exist? IMHO this should
> raise an error
> instead of using the default?
This sure needs handling but I would prefer a warning, when all other
keys for the connection are available in $params, then the connection
could still be done and default values would be taken. If other keys
would be missing, then the rados_connect would die later on.

>
> > pve_rados_conf_set($conn, 'client_mount_timeout', $timeout);
> >
> > foreach my $k (keys %params) {
> > diff --git a/RADOS.xs b/RADOS.xs
> > index a9f6bc3..ad3cf96 100644
> > --- a/RADOS.xs
> > +++ b/RADOS.xs
> > @@ -47,19 +47,35 @@ CODE:
> >  }
> >
> >  void
> > -pve_rados_connect(cluster)
> > +pve_rados_conf_read_file(cluster, path)
> >  rados_t cluster
> > -PROTOTYPE: $
> > +SV *path
> > +PROTOTYPE: $$
> >  CODE:
> >  {
> > -DPRINTF("pve_rados_connect\n");
> > +char *p = NULL;
> >
> > -int res = rados_conf_read_file(cluster, NULL);
> > +if (SvOK(path)) {
> > +   p = SvPV_nolen(path);
> > +}
> > +
> > +DPRINTF("pve_rados_conf_read_file %s\n", p);
> > +
> > +int res = rados_conf_read_file(cluster, p);
>
>
> I thought we only want to call this if p != NULL ?
I kept this to stay with the default behaviour of ceph and if there
is no config, then ceph searches:
- $CEPH_CONF (environment variable)
- /etc/ceph/ceph.conf
- ~/.ceph/config
- ceph.conf (in the current working directory)our current

Currently our code is also expecting the config under
/etc/ceph/ceph.conf, I tried to keep it similar to it.

>
> >  if (res < 0) {
> >  die("rados_conf_read_file failed - %s\n", strerror(-res));
> >  }
> > +}
> > +
> > +void
> > +pve_rados_connect(cluster)
> > +rados_t cluster
> > +PROTOTYPE: $
> > +CODE:
> > +{
> > +DPRINTF("pve_rados_connect\n");
> >
> > -res = rados_connect(cluster);
> > +int res = rados_connect(cluster);
> >  if (res < 0) {
> >  die("rados_connect failed - %s\n", strerror(-res));
> >  }
> > --
> > 2.11.0
> >
> >
> > ___
> > 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 librados2-perl] Split method pve_rados_connect

[Dietmar Maurer]

comments inline

> On March 30, 2018 at 12:25 PM Alwin Antreich  wrote:
> 
> 
> To be able to connect through librados2 without a config file, the
> method pve_rados_connect is split up into pve_rados_connect and
> pve_rados_conf_read_file.
> 
> Signed-off-by: Alwin Antreich 
> ---
>  PVE/RADOS.pm |  9 -
>  RADOS.xs | 26 +-
>  2 files changed, 29 insertions(+), 6 deletions(-)
> 
> diff --git a/PVE/RADOS.pm b/PVE/RADOS.pm
> index aa6a102..ad1c2db 100644
> --- a/PVE/RADOS.pm
> +++ b/PVE/RADOS.pm
> @@ -1,6 +1,6 @@
>  package PVE::RADOS;
>  
> -use 5.014002;
> +use 5.014002; # FIXME: update version??

why this FIXME?

>  use strict;
>  use warnings;
>  use Carp;
> @@ -13,6 +13,7 @@ use PVE::RPCEnvironment;
>  require Exporter;
>  
>  my $rados_default_timeout = 5;
> +my $ceph_default_conf = '/etc/ceph/ceph.conf';
>  
>  
>  our @ISA = qw(Exporter);
> @@ -164,6 +165,12 @@ sub new {
>   $conn = pve_rados_create() ||
>   die "unable to create RADOS object\n";
>  
> + my $ceph_conf = delete $params{ceph_conf} || $ceph_default_conf;
> +
> + if (-e $ceph_conf) {
> + pve_rados_conf_read_file($conn, $ceph_conf);
> + }
> +

What if $params{ceph_conf} is set, but file does not exist? IMHO this should
raise an error
instead of using the default?

>   pve_rados_conf_set($conn, 'client_mount_timeout', $timeout);
>  
>   foreach my $k (keys %params) {
> diff --git a/RADOS.xs b/RADOS.xs
> index a9f6bc3..ad3cf96 100644
> --- a/RADOS.xs
> +++ b/RADOS.xs
> @@ -47,19 +47,35 @@ CODE:
>  }
>  
>  void
> -pve_rados_connect(cluster) 
> +pve_rados_conf_read_file(cluster, path)
>  rados_t cluster
> -PROTOTYPE: $
> +SV *path
> +PROTOTYPE: $$
>  CODE:
>  {
> -DPRINTF("pve_rados_connect\n");
> +char *p = NULL;
>  
> -int res = rados_conf_read_file(cluster, NULL);
> +if (SvOK(path)) {
> + p = SvPV_nolen(path);
> +}
> +
> +DPRINTF("pve_rados_conf_read_file %s\n", p);
> +
> +int res = rados_conf_read_file(cluster, p);


I thought we only want to call this if p != NULL ?

>  if (res < 0) {
>  die("rados_conf_read_file failed - %s\n", strerror(-res));
>  }
> +}
> +
> +void
> +pve_rados_connect(cluster)
> +rados_t cluster
> +PROTOTYPE: $
> +CODE:
> +{
> +DPRINTF("pve_rados_connect\n");
>  
> -res = rados_connect(cluster);
> +int res = rados_connect(cluster);
>  if (res < 0) {
>  die("rados_connect failed - %s\n", strerror(-res));
>  }
> -- 
> 2.11.0
> 
> 
> ___
> 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 librados2-perl] Split method pve_rados_connect

[Thomas Lamprecht]

Am 03/30/2018 um 12:25 PM schrieb Alwin Antreich:

To be able to connect through librados2 without a config file, the
method pve_rados_connect is split up into pve_rados_connect and
pve_rados_conf_read_file.

Signed-off-by: Alwin Antreich 
---
  PVE/RADOS.pm |  9 -
  RADOS.xs | 26 +-
  2 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/PVE/RADOS.pm b/PVE/RADOS.pm
index aa6a102..ad1c2db 100644
--- a/PVE/RADOS.pm
+++ b/PVE/RADOS.pm
@@ -1,6 +1,6 @@
  package PVE::RADOS;
  
-use 5.014002;

+use 5.014002; # FIXME: update version??
  use strict;
  use warnings;
  use Carp;
@@ -13,6 +13,7 @@ use PVE::RPCEnvironment;
  require Exporter;
  
  my $rados_default_timeout = 5;

+my $ceph_default_conf = '/etc/ceph/ceph.conf';
  
  
  our @ISA = qw(Exporter);

@@ -164,6 +165,12 @@ sub new {
$conn = pve_rados_create() ||
die "unable to create RADOS object\n";
  
+	my $ceph_conf = delete $params{ceph_conf} || $ceph_default_conf;

+
+   if (-e $ceph_conf) {
+   pve_rados_conf_read_file($conn, $ceph_conf);
+   }
+
pve_rados_conf_set($conn, 'client_mount_timeout', $timeout);
  
  	foreach my $k (keys %params) {

diff --git a/RADOS.xs b/RADOS.xs
index a9f6bc3..ad3cf96 100644
--- a/RADOS.xs
+++ b/RADOS.xs
@@ -47,19 +47,35 @@ CODE:


This whole hunk does not apply here...
A quick look gave me one whitespace problem (see below), but that alone 
did not fix it for me...

Are you sure you sent all commits between this and origin/master ?

git log origin/master..



  }
  
  void

-pve_rados_connect(cluster)
+pve_rados_conf_read_file(cluster, path)
  rados_t cluster
-PROTOTYPE: $
+SV *path
+PROTOTYPE: $$
  CODE:
  {
-DPRINTF("pve_rados_connect\n");
+char *p = NULL;
  
-int res = rados_conf_read_file(cluster, NULL);

+if (SvOK(path)) {
+   p = SvPV_nolen(path);
+}
+
+DPRINTF("pve_rados_conf_read_file %s\n", p);
+
+int res = rados_conf_read_file(cluster, p);
  if (res < 0) {
  die("rados_conf_read_file failed - %s\n", strerror(-res));
  }
+}
+
+void
+pve_rados_connect(cluster)
+rados_t cluster
+PROTOTYPE: $
+CODE:
+{
+DPRINTF("pve_rados_connect\n");
  


The empty line above contains a trailing whitespace in origin/master, 
which your patch does not contain.



-res = rados_connect(cluster);
+int res = rados_connect(cluster);
  if (res < 0) {
  die("rados_connect failed - %s\n", strerror(-res));
  }



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

[pve-devel] [PATCH librados2-perl] Split method pve_rados_connect

[Alwin Antreich]

To be able to connect through librados2 without a config file, the
method pve_rados_connect is split up into pve_rados_connect and
pve_rados_conf_read_file.

Signed-off-by: Alwin Antreich 
---
 PVE/RADOS.pm |  9 -
 RADOS.xs | 26 +-
 2 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/PVE/RADOS.pm b/PVE/RADOS.pm
index aa6a102..ad1c2db 100644
--- a/PVE/RADOS.pm
+++ b/PVE/RADOS.pm
@@ -1,6 +1,6 @@
 package PVE::RADOS;
 
-use 5.014002;
+use 5.014002; # FIXME: update version??
 use strict;
 use warnings;
 use Carp;
@@ -13,6 +13,7 @@ use PVE::RPCEnvironment;
 require Exporter;
 
 my $rados_default_timeout = 5;
+my $ceph_default_conf = '/etc/ceph/ceph.conf';
 
 
 our @ISA = qw(Exporter);
@@ -164,6 +165,12 @@ sub new {
$conn = pve_rados_create() ||
die "unable to create RADOS object\n";
 
+   my $ceph_conf = delete $params{ceph_conf} || $ceph_default_conf;
+
+   if (-e $ceph_conf) {
+   pve_rados_conf_read_file($conn, $ceph_conf);
+   }
+
pve_rados_conf_set($conn, 'client_mount_timeout', $timeout);
 
foreach my $k (keys %params) {
diff --git a/RADOS.xs b/RADOS.xs
index a9f6bc3..ad3cf96 100644
--- a/RADOS.xs
+++ b/RADOS.xs
@@ -47,19 +47,35 @@ CODE:
 }
 
 void
-pve_rados_connect(cluster) 
+pve_rados_conf_read_file(cluster, path)
 rados_t cluster
-PROTOTYPE: $
+SV *path
+PROTOTYPE: $$
 CODE:
 {
-DPRINTF("pve_rados_connect\n");
+char *p = NULL;
 
-int res = rados_conf_read_file(cluster, NULL);
+if (SvOK(path)) {
+   p = SvPV_nolen(path);
+}
+
+DPRINTF("pve_rados_conf_read_file %s\n", p);
+
+int res = rados_conf_read_file(cluster, p);
 if (res < 0) {
 die("rados_conf_read_file failed - %s\n", strerror(-res));
 }
+}
+
+void
+pve_rados_connect(cluster)
+rados_t cluster
+PROTOTYPE: $
+CODE:
+{
+DPRINTF("pve_rados_connect\n");
 
-res = rados_connect(cluster);
+int res = rados_connect(cluster);
 if (res < 0) {
 die("rados_connect failed - %s\n", strerror(-res));
 }
-- 
2.11.0


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