[libvirt] [PATCH] rbd: Do not append Ceph monitor port number 6789 if not provided

[John Ferlan]

On 01/06/2016 05:36 AM, Wido den Hollander wrote:
> If no port number was provided for a storage pool libvirt defaults to
> port 6789; however, librbd/librados already default to 6789 when no port
> number is provided.
> 
> In the future Ceph will switch to a new port for the Ceph monitors since
> port 6789 is already assigned to a different application by IANA.
> 
> Port 6789 is assigned to SMC-HTTPS and Ceph now has port 3300 assigned as
> the 'Ceph monitor' port.
> 
> In this case it is the best solution to not hardcode any port number into
> libvirt and let librados handle the connection.
> 
> Only if a user specifies a different port number we pass it down to librados,
> otherwise we leave it blank.
> 
> Signed-off-by: Wido den Hollander <wido at widodh.nl>
> ---
>  .gnulib                           | 2 +-
>  docs/formatdomain.html.in         | 2 +-
>  docs/storage.html.in              | 8 +++++---
>  src/storage/storage_backend_rbd.c | 2 +-
>  src/util/virstoragefile.c         | 3 +--
>  5 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/.gnulib b/.gnulib
> index 6cc32c6..f39477d 160000
> --- a/.gnulib
> +++ b/.gnulib
> @@ -1 +1 @@
> -Subproject commit 6cc32c63e80bc1a30c521b2f07f2b54909b59892
> +Subproject commit f39477dba778e99392948dd3dd19ec0d46aee932

You may want to "git submodule update --init --recursive"  ;-)

> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index ce46f06..889e721 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -2151,7 +2151,7 @@
>                  <td> rbd </td>
>                  <td> monitor servers of RBD </td>
>                  <td> one or more </td>
> -                <td> 6789 </td>
> +                <td> librados default </td>
>                </tr>
>                <tr>
>                  <td> sheepdog </td>
> diff --git a/docs/storage.html.in b/docs/storage.html.in
> index 6c8222a..1f01330 100644
> --- a/docs/storage.html.in
> +++ b/docs/storage.html.in
> @@ -550,7 +550,9 @@
>        backend supports cephx authentication for communication with the
>        Ceph cluster. Storing the cephx authentication key is done with
>        the libvirt secret mechanism. The UUID in the example pool input
> -      refers to the UUID of the stored secret.
> +      refers to the UUID of the stored secret.<br />
> +      The port attribute for a Ceph monitor does not have to be provided.
> +      If not provided librados will use the default Ceph monitor port.
>        <span class="since">Since 0.9.13</span>
>      </p>
>  
> @@ -560,8 +562,8 @@
>          <name>myrbdpool</name>
>          <source>
>            <name>rbdpool</name>
> -          <host name='1.2.3.4' port='6789'/>
> -          <host name='my.ceph.monitor' port='6789'/>
> +          <host name='1.2.3.4'/>
> +          <host name='my.ceph.monitor'/>
>            <host name='third.ceph.monitor' port='6789'/>
>            <auth username='admin' type='ceph'>
>              <secret uuid='2ec115d7-3a88-3ceb-bc12-0ac909a6fd87'/>
> diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c
> index 80684eb..3235a3e 100644
> --- a/src/storage/storage_backend_rbd.c
> +++ b/src/storage/storage_backend_rbd.c
> @@ -173,7 +173,7 @@ static int virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr,
>      for (i = 0; i < source->nhost; i++) {
>          if (source->hosts[i].name != NULL &&
>              !source->hosts[i].port) {
> -            virBufferAsprintf(&mon_host, "%s:6789,",
> +            virBufferAsprintf(&mon_host, "%s,",
>                                source->hosts[i].name);
>          } else if (source->hosts[i].name != NULL &&
>              source->hosts[i].port) {
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index 2aa1d90..f6ba0c8 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -2246,8 +2246,7 @@ virStorageSourceRBDAddHost(virStorageSourcePtr src,
>          if (VIR_STRDUP(src->hosts[src->nhosts - 1].port, port) < 0)
>              goto error;
>      } else {
> -        if (VIR_STRDUP(src->hosts[src->nhosts - 1].port, "6789") < 0)
> -            goto error;
> +        src->hosts[src->nhosts - 1].port = NULL;

I think perhaps somewhat ironically that since the allocation already
does a memset of 0 on the new 'hosts' element (see virExpandN), so
setting to NULL (or VIR_STRDUP'ing NULL) was superfluous.

I've made the obvious adjustment and pushed.

Tks -

John
>      }
>  
>      parts = virStringSplit(hostport, "\\:", 0);
>