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.antre...@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.antre...@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@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