[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 <a.antreich at proxmox.com> 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 <a.antreich at proxmox.com>
> > ---
> >  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 at pve.proxmox.com
> > https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel