Re: [libvirt] [PATCH v2 1/5] storage: util: Add boolean differentiating between gluster lookup type

3 Apr 2017


      On Thu, 2017-03-30 at 17:12 +0200, Peter Krempa wrote:
...
The native gluster pool source list data differs from the data used for
attaching gluster volumes as
netfs pools. Currently the only difference
was the format. Since native pools don't use it and later there will be
more difference add a boolean to swithc
between the types instead.
s/difference/differences/
s/swithc/switch/

[...]
...
@@ -2839,18 +2839,21 @@ virStorageBackendDeleteLocal(virConnectPtr conn ATTRIBUTE_UNUSED,
  /**
   * virStorageBackendFindGlusterPoolSources:
   * @host: host to detect volumes on
- * @pooltype: src->format is set to this value
   * @list: list of storage pool sources to be filled
+ * @netfs: lookup will be used with netfs pools
   * @report: report error if the 'gluster' cli tool is missing
   *
   * Looks up gluster volumes on @host and fills them to @list.
   *
+ * If @netfs is specified the data is tweaked so that it can be used with netfs
+ * type pools. Otherwise the data is for use with native gluster pools.
+ *
   * Returns number of volumes on the host on success, or -1 on error.
   */
  int
  virStorageBackendFindGlusterPoolSources(const char *host,
-                                        int pooltype,
                                          virStoragePoolSourceListPtr list,
+                                        bool netfs,
I suggest using virStoragePoolType instead of bool here, eg.
callers will pass either VIR_STORAGE_POOL_GLUSTER for native
gluster pools or VIR_STORAGE_POOL_NETFS for netfs pools.
Passing any other value in the enumeration would of course
result in an error.

This would make the calling sites less opaque.

[...]
...
@@ -2918,7 +2921,8 @@ virStorageBackendFindGlusterPoolSources(const char *host,
          if (VIR_STRDUP(src->hosts[0].name, host) < 0)
              goto cleanup;
 
-        src->format = pooltype;
+        if (netfs)
+            src->format = VIR_STORAGE_POOL_NETFS_GLUSTERFS;
In patch 5/5 you're going to move this chunk of code earlier
in the function while changing it: I suggest you move it in
this patch instead, so there's only a single code motion
instead of two.


ACK with the above suggestions addressed, or if you manage
to convince me that they're best left unaddressed :)

-- 
Andrea Bolognani / Red Hat / Virtualization

    