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

[Dietmar Maurer]

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?

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