Re: [PATCHv2] storage: Fix logical pool fmt type

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Thank you.

On 09/29/2014 01:53 PM, John Ferlan wrote:


On 09/26/2014 06:56 AM, John Ferlan wrote:


On 09/25/2014 10:26 AM, Erik Skultety wrote:
According to our documentation logical pool supports formats 'auto' and
'lvm2'. However, in storage_conf.c we prevously defined storage pool

s/prevously/previously

formats: unknown, lvm2. Due to backward compatibility reasons
documentation now refers to pool format type 'unknown' instead of 'auto'.

could be modified depending on how you handle my comment below.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1123767
---
  docs/schemas/storagepool.rng | 2 +-
  docs/storage.html.in         | 2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng
index 2d165a3..7234ef3 100644
--- a/docs/schemas/storagepool.rng
+++ b/docs/schemas/storagepool.rng
@@ -465,7 +465,7 @@
        <element name='format'>
          <attribute name='type'>
            <choice>
-            <value>auto</value>
+            <value>unknown</value>

Perhaps in order to avoid someone "in the future" getting us back into
this mess - can we add a comment after the </value>:

"<!-- back-compat requires keeping 'unknown' not 'auto' -->"

There's a few other examples of back-compat comments...

              <value>lvm2</value>
            </choice>
          </attribute>
diff --git a/docs/storage.html.in b/docs/storage.html.in
index 3d2ffca..49fd862 100644
--- a/docs/storage.html.in
+++ b/docs/storage.html.in
@@ -331,7 +331,7 @@
        The logical volume pool supports the following formats:
      </p>
      <ul>
-      <li><code>auto</code> - automatically determine format</li>
+      <li><code>unknown</code> - automatically determine format</li>

I think if you follow what 'virStoragePoolFormatDisk' does (or Disk
volume pools on the webpage) and just don't list 'unknown' that'd
probably be better.  Unless someone else thinks it should be listed.
Yes, a list of 1 element looks strange.  If that's not desired some text
indicating that logical pools only support the 'lvm2' type and if format
is not provided, then libvirt will determine the type.


        <li>
          <code>lvm2</code>
        </li>


ACK

Let's see if anyone else has feelings one way or another - I can modify
based on my review and push so you don't have to send a v3.  Just want
to give others a chance first...

John

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list


I pushed the following for you

John


diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng
index 2d165a3..0f05c5c 100644
--- a/docs/schemas/storagepool.rng
+++ b/docs/schemas/storagepool.rng
@@ -465,7 +465,7 @@
        <element name='format'>
          <attribute name='type'>
            <choice>
-            <value>auto</value>
+            <value>unknown</value> <!-- back-compat requires keeping 'unknown' not 'auto' -->
              <value>lvm2</value>
            </choice>
          </attribute>
diff --git a/docs/storage.html.in b/docs/storage.html.in
index 3d2ffca..9933548 100644
--- a/docs/storage.html.in
+++ b/docs/storage.html.in
@@ -328,14 +328,10 @@

      <h3>Valid pool format types</h3>
      <p>
-      The logical volume pool supports the following formats:
+      The logical volume pool supports only the <code>lvm2</code> format,
+      although not supplying a format value will result in automatic
+      selection of the<code>lvm2</code> format.
      </p>
-    <ul>
-      <li><code>auto</code> - automatically determine format</li>
-      <li>
-        <code>lvm2</code>
-      </li>
-    </ul>

      <h3>Valid volume format types</h3>
      <p>


--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]