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

Re: [libvirt] [PATCH v2 1/4] conf: add rawio attribute to disk element of domain XML



On 01/31/2012 07:03 AM, Daniel P. Berrange wrote:
On Tue, Jan 31, 2012 at 01:49:11PM +0900, Taku Izumi wrote:
@@ -3156,6 +3159,26 @@ virDomainDiskDefParseXML(virCapsPtr caps
          def->snapshot = VIR_DOMAIN_DISK_SNAPSHOT_NO;
      }

+    def->rawio = -1; /* unspecified */
+    if (rawio) {
+        if (def->device == VIR_DOMAIN_DISK_DEVICE_LUN) {
+            if (STREQ(rawio, "yes")) {
+                def->rawio = 1;
+            } else if (STREQ(rawio, "no")) {
+                def->rawio = 0;
+            } else {
+                virDomainReportError(VIR_ERR_INTERNAL_ERROR,
+                                     _("unknown disk rawio setting '%s'"),
+                                     rawio);
+                goto error;
+            }
+        } else {
+            virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                                _("rawio can be used only with device='lun'"));
s/INTERNAL_ERROR/XML_ERROR/


ACK with that change

I've pushed this patch with that, and some other, changes (see attached diff for all changes aside from what you pointed out - I managed to squash that in before I thought to save a diff).

Just now I realize I had misinterpreted your comment as being only about the first of those two messages (my brain stopped looking at the email as soon as I hit the first one - you should have done "s/.../.../g" :-), but when I looked at the file I saw the 2nd, I decided it should be VIR_ERR_CONFIG_UNSUPPORTED. Should I push a change to XML_ERROR, or leave it as is? (I *still* get confused about which is more appropriate where, but this one is in kind of a gray area.)

I also found that "make check wouldn't pass, which was mostly traced to the concept of making the default value of rawio "-1". The problem with this is that there are other functions that create and fill-in domain structures, and they hadn't been taught about this default value. I changed the object to have a "bool rawio_specified", and modified the parse and format functions accordingly.

Also there was no test that exercised the rawio attribute - I added that into the virtio-lun test.

(and just now, I realize that I forgot to add a blurb in the docs. I'll do that when I'm done with these emails)

Also change default of rawio from -1 to 0, and add rawio_specified
bool to the object to differentiate between being unset, and setting
it explicitly to 0. Prior to this, it would fail make check.
---
 src/conf/domain_conf.c                             |   12 +++++++-----
 src/conf/domain_conf.h                             |    3 ++-
 .../qemuxml2argvdata/qemuxml2argv-virtio-lun.args  |    6 +++---
 tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.xml |   13 ++++++-------
 4 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 4f13ac2..35cb7a4 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3159,8 +3159,8 @@ virDomainDiskDefParseXML(virCapsPtr caps,
         def->snapshot = VIR_DOMAIN_DISK_SNAPSHOT_NO;
     }
 
-    def->rawio = -1; /* unspecified */
     if (rawio) {
+        def->rawio_specified = true;
         if (def->device == VIR_DOMAIN_DISK_DEVICE_LUN) {
             if (STREQ(rawio, "yes")) {
                 def->rawio = 1;
@@ -9995,10 +9995,12 @@ virDomainDiskDefFormat(virBufferPtr buf,
     virBufferAsprintf(buf,
                       "    <disk type='%s' device='%s'",
                       type, device);
-    if (def->rawio == 1) {
-        virBufferAddLit(buf, " rawio='yes'");
-    } else if (def->rawio == 0) {
-        virBufferAddLit(buf, " rawio='no'");
+    if (def->rawio_specified) {
+        if (def->rawio == 1) {
+            virBufferAddLit(buf, " rawio='yes'");
+        } else if (def->rawio == 0) {
+            virBufferAddLit(buf, " rawio='no'");
+        }
     }
     if (def->snapshot &&
         !(def->snapshot == VIR_DOMAIN_DISK_SNAPSHOT_NO && def->readonly))
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index b01f0b1..503684f 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -413,7 +413,8 @@ struct _virDomainDiskDef {
     unsigned int transient : 1;
     virDomainDeviceInfo info;
     virStorageEncryptionPtr encryption;
-    int rawio; /* unspecified = -1, no = 0, yes = 1 */
+    bool rawio_specified;
+    int rawio; /* no = 0, yes = 1 */
 };
 
 
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.args b/tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.args
index 5df9182..b229f2a 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.args
@@ -4,8 +4,8 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \
 -boot dc -device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x6 \
 -drive file=/dev/sdfake,if=none,id=drive-virtio-disk0 \
 -device virtio-blk-pci,scsi=on,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,id=virtio-disk0 \
--drive file=/var/lib/libvirt/Fedora-14-x86_64-Live-KDE.iso,if=none,media=cdrom,id=drive-ide0-1-0 \
--device ide-drive,bus=ide.1,unit=0,drive=drive-ide0-1-0,id=ide0-1-0 \
+-drive file=/dev/sdfake2,if=none,id=drive-virtio-disk1 \
+-device virtio-blk-pci,scsi=on,bus=pci.0,addr=0x5,drive=drive-virtio-disk1,id=virtio-disk1 \
 -device virtio-net-pci,vlan=0,id=net0,mac=52:54:00:e5:48:58,bus=pci.0,addr=0x3 \
 -net user,vlan=0,name=hostnet0 -serial pty -usb -vnc 127.0.0.1:-809 -std-vga \
--device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x7
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.xml b/tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.xml
index abe1b2f..63107d5 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.xml
+++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.xml
@@ -16,18 +16,17 @@
   <on_crash>restart</on_crash>
   <devices>
     <emulator>/usr/bin/qemu</emulator>
-    <disk type='block' device='lun'>
+    <disk type='block' device='lun' rawio='yes'>
       <driver name='qemu' type='qcow2'/>
       <source dev='/dev/sdfake'/>
       <target dev='vda' bus='virtio'/>
       <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
     </disk>
-    <disk type='file' device='cdrom'>
-      <driver name='qemu' type='raw'/>
-      <source file='/var/lib/libvirt/Fedora-14-x86_64-Live-KDE.iso'/>
-      <target dev='hdc' bus='ide'/>
-      <readonly/>
-      <address type='drive' controller='0' bus='1' unit='0'/>
+    <disk type='block' device='lun'>
+      <driver name='qemu' type='qcow2'/>
+      <source dev='/dev/sdfake2'/>
+      <target dev='vdb' bus='virtio'/>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/>
     </disk>
     <controller type='virtio-serial' index='0'>
       <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/>
-- 
1.7.7.5


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