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

[Wido den Hollander]

On 06-01-16 14:55, John Ferlan wrote:
> 
> 
> 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"  ;-)
> 

Argh! That one got in during my rebase. My apologies.

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

Super, tnx!

Wido

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