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

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(a)widodh.nl&gt; > --- > .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 @@ > &lt;name&gt;myrbdpool&lt;/name&gt; > &lt;source&gt; > &lt;name&gt;rbdpool&lt;/name&gt; > - &lt;host name='1.2.3.4' port='6789'/&gt; > - &lt;host name='my.ceph.monitor' port='6789'/&gt; > + &lt;host name='1.2.3.4'/&gt; > + &lt;host name='my.ceph.monitor'/&gt; > &lt;host name='third.ceph.monitor' port='6789'/&gt; > &lt;auth username='admin' type='ceph'&gt; > &lt;secret uuid='2ec115d7-3a88-3ceb-bc12-0ac909a6fd87'/&gt; > 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); >