Re: [libvirt] [PATCHv2] xml: introduce startupPolicy for chardev device

2013-05-28 Thread Eric Blake 
On 05/24/2013 04:46 PM, Eric Blake wrote:
 From: Seiji Aguchi seiji.agu...@hds.com
 
 [Problem]
 Currently, guest OS's messages can be logged to a local disk of host OS
 by creating chadevs with options below.
   -chardev file,id=charserial0,path=log file's path -device 
 isa-serial,chardev=chardevserial0,id=serial0

 
 Seiji's patch plus my initial review comments; I've ack'd this much,
 but want to add a round-trip xml2xml test before pushing anything
 (Seiji, if you want to use this as a starting point, rather than
 waiting for my long weekend, feel free to post a v3).


Sorry, but this needs a v3, and I'm going to hand it back to you to fix.
 I tried adding a testsuite, but it proved that you weren't outputting
the startupPolicy during dumpxml formatting.  My initial attempt to add
it backfired (I'll post it below); I'm worried that we are playing too
fast and loose with a union type, since even with my patch in place,
'make check' fails with problems like:

84) QEMU XML-2-XML serial-dev ...
Offset 913
Expect [/
  target port='0'/
/serial
console type='dev'
  source path='/dev/ttyS2]
Actual [ startupPolicy='(null)'/
  target port='0'/
/serial
console type='dev'
  source path='/dev/ttyS2' startupPolicy='(null)]

which is a bug, since you said startupPolicy should only be tied to
files and not other serial types.  Do we need to repeat the
startupPolicy on the console mirror of the first serial device, or
is that unnecessary back-compat, and where we can safely declare that
startupPolicy is only emitted for the serial side of things?

All told, your patch is not complete until it passes 'make check' with a
new xml2xml test that proves we can round trip the new policy.

diff --git c/src/conf/domain_conf.c w/src/conf/domain_conf.c
index c24a9f0..5af5e40 100644
--- c/src/conf/domain_conf.c
+++ w/src/conf/domain_conf.c
@@ -14547,8 +14547,14 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
 if (def-type != VIR_DOMAIN_CHR_TYPE_PTY ||
 (def-data.file.path 
  !(flags  VIR_DOMAIN_XML_INACTIVE))) {
-virBufferEscapeString(buf,   source path='%s'/\n,
+virBufferEscapeString(buf,   source path='%s',
   def-data.file.path);
+
+if (def-data.file.path  def-data.file.startupPolicy) {
+const char *policy =
virDomainStartupPolicyTypeToString(def-data.file.startupPolicy);
+virBufferAsprintf(buf,  startupPolicy='%s', policy);
+}
+virBufferAddLit(buf, /\n);
 }
 break;

diff --git
c/tests/qemuxml2argvdata/qemuxml2argv-serial-source-optional.args
w/tests/qemuxml2argvdata/qemuxml2argv-serial-source-optional.args
new file mode 100644
index 000..24977f2
--- /dev/null
+++ w/tests/qemuxml2argvdata/qemuxml2argv-serial-source-optional.args
@@ -0,0 +1,8 @@
+LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu
-S -M \
+pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults \
+-chardev socket,id=charmonitor,path=/tmp/test-monitor,server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=readline \
+-no-acpi -boot c -usb -hdc /tmp/idedisk.img \
+-chardev file,id=charserial0,path=/tmp/serial.log \
+-device isa-serial,chardev=charserial0,id=serial0 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git
c/tests/qemuxml2argvdata/qemuxml2argv-serial-source-optional.xml
w/tests/qemuxml2argvdata/qemuxml2argv-serial-source-optional.xml
new file mode 100644
index 000..34e0eb9
--- /dev/null
+++ w/tests/qemuxml2argvdata/qemuxml2argv-serial-source-optional.xml
@@ -0,0 +1,35 @@
+domain type='qemu'
+  nameQEMUGuest1/name
+  uuidc7a5fdbd-edaf-9455-926a-d65c16db1809/uuid
+  memory unit='KiB'219136/memory
+  currentMemory unit='KiB'219136/currentMemory
+  vcpu placement='static'1/vcpu
+  os
+type arch='i686' machine='pc'hvm/type
+boot dev='hd'/
+  /os
+  clock offset='utc'/
+  on_poweroffdestroy/on_poweroff
+  on_rebootrestart/on_reboot
+  on_crashdestroy/on_crash
+  devices
+emulator/usr/bin/qemu/emulator
+disk type='file' device='disk'
+  source file='/tmp/idedisk.img'/
+  target dev='hdc' bus='ide'/
+  address type='drive' controller='0' bus='0' target='0' unit='2'/
+/disk
+controller type='usb' index='0'/
+controller type='ide' index='0'/
+controller type='pci' index='0' model='pci-root'/
+serial type='file'
+  source path='/tmp/serial.log' startupPolicy='optional'/
+  target port='0'/
+/serial
+console type='file'
+  source path='/tmp/serial.log'/
+  target type='serial' port='0'/
+/console
+memballoon model='virtio'/
+  /devices
+/domain
diff --git c/tests/qemuxml2argvtest.c w/tests/qemuxml2argvtest.c
index 2c7fd01..22f4782 100644
--- c/tests/qemuxml2argvtest.c
+++ w/tests/qemuxml2argvtest.c
@@ -727,6 +727,8 @@ mymain(void)
 QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE,

Re: [libvirt] [PATCHv2] xml: introduce startupPolicy for chardev device

2013-05-28 Thread Seiji Aguchi 
Eric,


 -Original Message-
 From: libvir-list-boun...@redhat.com [mailto:libvir-list-boun...@redhat.com] 
 On Behalf Of Eric Blake
 Sent: Tuesday, May 28, 2013 7:08 PM
 Cc: libvir-list@redhat.com; dle-deve...@lists.sourceforge.net; Tomoki Sekiyama
 Subject: Re: [libvirt] [PATCHv2] xml: introduce startupPolicy for chardev 
 device
 
 On 05/24/2013 04:46 PM, Eric Blake wrote:
  From: Seiji Aguchi seiji.agu...@hds.com
 
  [Problem]
  Currently, guest OS's messages can be logged to a local disk of host OS
  by creating chadevs with options below.
-chardev file,id=charserial0,path=log file's path -device 
  isa-serial,chardev=chardevserial0,id=serial0
 
 
  Seiji's patch plus my initial review comments; I've ack'd this much,
  but want to add a round-trip xml2xml test before pushing anything
  (Seiji, if you want to use this as a starting point, rather than
  waiting for my long weekend, feel free to post a v3).
 
 
 Sorry, but this needs a v3, and I'm going to hand it back to you to fix.
  I tried adding a testsuite, but it proved that you weren't outputting
 the startupPolicy during dumpxml formatting.  My initial attempt to add
 it backfired (I'll post it below); 

Thank you for handling/testing my patch.

I'm worried that we are playing too
 fast and loose with a union type, since even with my patch in place,
 'make check' fails with problems like:
 
 84) QEMU XML-2-XML serial-dev ...
 Offset 913
 Expect [/
   target port='0'/
 /serial
 console type='dev'
   source path='/dev/ttyS2]
 Actual [ startupPolicy='(null)'/
   target port='0'/
 /serial
 console type='dev'
   source path='/dev/ttyS2' startupPolicy='(null)]
 
 which is a bug, since you said startupPolicy should only be tied to
 files and not other serial types.  Do we need to repeat the
 startupPolicy on the console mirror of the first serial device, or
 is that unnecessary back-compat, and where we can safely declare that
 startupPolicy is only emitted for the serial side of things?
 

I need to think carefully about it before deciding it in a hurry.

 All told, your patch is not complete until it passes 'make check' with a
 new xml2xml test that proves we can round trip the new policy.

Anyway, I will post the v3 patch by fixing the issue.

Seiji

 
 diff --git c/src/conf/domain_conf.c w/src/conf/domain_conf.c
 index c24a9f0..5af5e40 100644
 --- c/src/conf/domain_conf.c
 +++ w/src/conf/domain_conf.c
 @@ -14547,8 +14547,14 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
  if (def-type != VIR_DOMAIN_CHR_TYPE_PTY ||
  (def-data.file.path 
   !(flags  VIR_DOMAIN_XML_INACTIVE))) {
 -virBufferEscapeString(buf,   source path='%s'/\n,
 +virBufferEscapeString(buf,   source path='%s',
def-data.file.path);
 +
 +if (def-data.file.path  def-data.file.startupPolicy) {
 +const char *policy =
 virDomainStartupPolicyTypeToString(def-data.file.startupPolicy);
 +virBufferAsprintf(buf,  startupPolicy='%s', policy);
 +}
 +virBufferAddLit(buf, /\n);
  }
  break;
 
 diff --git
 c/tests/qemuxml2argvdata/qemuxml2argv-serial-source-optional.args
 w/tests/qemuxml2argvdata/qemuxml2argv-serial-source-optional.args
 new file mode 100644
 index 000..24977f2
 --- /dev/null
 +++ w/tests/qemuxml2argvdata/qemuxml2argv-serial-source-optional.args
 @@ -0,0 +1,8 @@
 +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu
 -S -M \
 +pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults \
 +-chardev socket,id=charmonitor,path=/tmp/test-monitor,server,nowait \
 +-mon chardev=charmonitor,id=monitor,mode=readline \
 +-no-acpi -boot c -usb -hdc /tmp/idedisk.img \
 +-chardev file,id=charserial0,path=/tmp/serial.log \
 +-device isa-serial,chardev=charserial0,id=serial0 \
 +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
 diff --git
 c/tests/qemuxml2argvdata/qemuxml2argv-serial-source-optional.xml
 w/tests/qemuxml2argvdata/qemuxml2argv-serial-source-optional.xml
 new file mode 100644
 index 000..34e0eb9
 --- /dev/null
 +++ w/tests/qemuxml2argvdata/qemuxml2argv-serial-source-optional.xml
 @@ -0,0 +1,35 @@
 +domain type='qemu'
 +  nameQEMUGuest1/name
 +  uuidc7a5fdbd-edaf-9455-926a-d65c16db1809/uuid
 +  memory unit='KiB'219136/memory
 +  currentMemory unit='KiB'219136/currentMemory
 +  vcpu placement='static'1/vcpu
 +  os
 +type arch='i686' machine='pc'hvm/type
 +boot dev='hd'/
 +  /os
 +  clock offset='utc'/
 +  on_poweroffdestroy/on_poweroff
 +  on_rebootrestart/on_reboot
 +  on_crashdestroy/on_crash
 +  devices
 +emulator/usr/bin/qemu/emulator
 +disk type='file' device='disk'
 +  source file='/tmp/idedisk.img'/
 +  target dev='hdc' bus='ide'/
 +  address type='drive' controller='0' bus='0' target='0' unit='2'/
 +/disk
 +controller type='usb' index='0

[libvirt] [PATCHv2] xml: introduce startupPolicy for chardev device

2013-05-24 Thread Eric Blake 
From: Seiji Aguchi seiji.agu...@hds.com

[Problem]
Currently, guest OS's messages can be logged to a local disk of host OS
by creating chadevs with options below.
  -chardev file,id=charserial0,path=log file's path -device 
isa-serial,chardev=chardevserial0,id=serial0

When a hardware failure happens in the disk, qemu-kvm can't create the
chardevs. In this case, guest OS doesn't boot up.

Actually, there are users who don't desire that guest OS goes down due
to a hardware failure of a log disk only. Therefore, qemu should offer
some way to boot guest OS up even if the log disk is broken.

[Solution]
This patch supports startupPolicy for chardev.

The starupPolicy is introduced just in cases where chardev is file
because this patch aims for making guest OS boot up when a hardware
failure happens.

In other cases (pty, dev, pipe and unix) it is not introduced
because they don't access to hardware.

The policy works as follows.
  - If the value is optional, guestOS boots up by dropping the chardev.
  - If other values are specified, guestOS fails to boot up. (the default)

Description about original startupPolicy attribute:
http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=e5a84d74a278

Signed-off-by: Seiji Aguchi seiji.agu...@hds.com
Signed-off-by: Eric Blake ebl...@redhat.com
---

Seiji's patch plus my initial review comments; I've ack'd this much,
but want to add a round-trip xml2xml test before pushing anything
(Seiji, if you want to use this as a starting point, rather than
waiting for my long weekend, feel free to post a v3).

 docs/formatdomain.html.in | 15 ++-
 docs/schemas/domaincommon.rng |  3 +++
 src/conf/domain_conf.c|  8 
 src/conf/domain_conf.h|  1 +
 src/qemu/qemu_process.c   | 23 ---
 tests/virt-aa-helper-test |  3 +++
 6 files changed, 45 insertions(+), 8 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 755d084..299fa49 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -4212,13 +4212,26 @@ qemu-kvm -net nic,model=? /dev/null
 p
   A file is opened and all data sent to the character
   device is written to the file.
+  span class=sinceSince 1.0.6/span, it is possible to define
+  policy on what happens if the file is not accessible when
+  booting or migrating. This is done by
+  a codestartupPolicy/code attribute:
 /p
+ul
+  liIf the value is mandatory (the default), the guest fails
+  to boot or migrate if the file is not found./li
+  liIf the value is optional, a missing file is at boot or
+  migration is substituted with /dev/null, so the guest still sees
+  the device but the host no longer tracks guest data on the device./li
+  liIf the value is requisite, the file is required for
+  booting, but optional on migration./li
+/ul

 pre
   ...
   lt;devicesgt;
 lt;serial type=filegt;
-  lt;source path=/var/log/vm/vm-serial.log/gt;
+  lt;source path=/var/log/vm/vm-serial.log startupPolicy=optional/gt;
   lt;target port=1/gt;
 lt;/serialgt;
   lt;/devicesgt;
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 3cace35..e8eec6c 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -2780,6 +2780,9 @@
 /optional
 optional
   attribute name=path/
+optional
+  ref name='startupPolicy'/
+/optional
 /optional
 optional
   attribute name=host/
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index a9656af..c24a9f0 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6626,6 +6626,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr 
def,
 char *path = NULL;
 char *mode = NULL;
 char *protocol = NULL;
+char *startupPolicy = NULL;
 int remaining = 0;

 while (cur != NULL) {
@@ -6646,6 +6647,9 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr 
def,
  !(flags  VIR_DOMAIN_XML_INACTIVE)))
 path = virXMLPropString(cur, path);

+if (startupPolicy == NULL 
+def-type == VIR_DOMAIN_CHR_TYPE_FILE)
+startupPolicy = virXMLPropString(cur, startupPolicy);
 break;

 case VIR_DOMAIN_CHR_TYPE_UDP:
@@ -6718,6 +6722,10 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr 
def,

 def-data.file.path = path;
 path = NULL;
+
+def-data.file.startupPolicy =
+virDomainStartupPolicyTypeFromString(startupPolicy);
+startupPolicy = NULL;
 break;

 case VIR_DOMAIN_CHR_TYPE_STDIO:
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 3a71d6c..4d49dd5 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1074,6 +1074,7 @@ struct _virDomainChrSourceDef {
 /* no source for