12:57 a.m.
This two patches are trying to add support for qemu virtual device
usb-serial.
Add an optional 'type' attribute to <target> element of serial port
device. There are two choices for its value, 'isa-serial' and
'usb-serial'. For backward compatibility, when attribute 'type' is
missing the 'isa-serial' will be chose as before.
Libvirt XML sample
<serial type='pty'>
<target type='usb-serial' port='0'/>
<address type='usb' bus='0' port='1'/>
</serial>
qemu commandline:
qemu ${other_vm_args} \
-chardev pty,id=charserial0 \
-device usb-serial,chardev=charserial0,id=serial0,bus=usb.0,port=1
Guannan Ren(2)
[PATCH 1/2] qemu: add usb-serial caps flag
[PATCH 2/2] qemu: add usb-serial support
docs/formatdomain.html.in | 9 ++++++++-
docs/schemas/domaincommon.rng | 20 +++++++++++++++++---
src/conf/domain_conf.c | 42 +++++++++++++++++++++++++++++++++++++++---
src/conf/domain_conf.h | 10 ++++++++++
src/libvirt_private.syms | 2 ++
src/qemu/qemu_capabilities.c | 2 ++
src/qemu/qemu_capabilities.h | 1 +
src/qemu/qemu_command.c | 24 ++++++++++++++++++++++--
tests/qemuhelptest.c | 21 ++++++++++++++-------
9 files changed, 115 insertions(+), 16 deletions(-)
12:57 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: add usb-serial caps flag
QEMU_CAPS_DEVICE_USB_SERIAL /* -device usb-serial */
---
src/qemu/qemu_capabilities.c | 2 ++
src/qemu/qemu_capabilities.h | 1 +
tests/qemuhelptest.c | 21 ++++++++++++++-------
3 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 17e6679..1fe9d07 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -199,6 +199,7 @@ VIR_ENUM_IMPL(qemuCaps, QEMU_CAPS_LAST,
"cirrus-vga",
"vmware-svga",
"device-video-primary",
+ "usb-serial",
);
struct _qemuCaps {
@@ -1343,6 +1344,7 @@ struct qemuCapsStringFlags qemuCapsObjectTypes[] = {
{ "VGA", QEMU_CAPS_DEVICE_VGA },
{ "cirrus-vga", QEMU_CAPS_DEVICE_CIRRUS_VGA },
{ "vmware-svga", QEMU_CAPS_DEVICE_VMWARE_SVGA },
+ { "usb-serial", QEMU_CAPS_DEVICE_USB_SERIAL},
};
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 3457852..d939821 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -162,6 +162,7 @@ enum qemuCapsFlags {
QEMU_CAPS_DEVICE_VMWARE_SVGA = 122, /* -device vmware-svga */
QEMU_CAPS_DEVICE_VIDEO_PRIMARY = 123, /* safe to use -device XXX
for primary video device */
+ QEMU_CAPS_DEVICE_USB_SERIAL = 124, /* -device usb-serial */
QEMU_CAPS_LAST, /* this must always be the last item */
};
diff --git a/tests/qemuhelptest.c b/tests/qemuhelptest.c
index 252ad3a..b71c292 100644
--- a/tests/qemuhelptest.c
+++ b/tests/qemuhelptest.c
@@ -395,7 +395,8 @@ mymain(void)
QEMU_CAPS_DEVICE_QXL,
QEMU_CAPS_DEVICE_VGA,
QEMU_CAPS_DEVICE_CIRRUS_VGA,
- QEMU_CAPS_DEVICE_VMWARE_SVGA);
+ QEMU_CAPS_DEVICE_VMWARE_SVGA,
+ QEMU_CAPS_DEVICE_USB_SERIAL);
DO_TEST("qemu-kvm-0.12.3", 12003, 1, 0,
QEMU_CAPS_VNC_COLON,
QEMU_CAPS_NO_REBOOT,
@@ -502,7 +503,8 @@ mymain(void)
QEMU_CAPS_DEVICE_QXL,
QEMU_CAPS_DEVICE_VGA,
QEMU_CAPS_DEVICE_CIRRUS_VGA,
- QEMU_CAPS_DEVICE_VMWARE_SVGA);
+ QEMU_CAPS_DEVICE_VMWARE_SVGA,
+ QEMU_CAPS_DEVICE_USB_SERIAL);
DO_TEST("qemu-kvm-0.12.1.2-rhel61", 12001, 1, 0,
QEMU_CAPS_VNC_COLON,
QEMU_CAPS_NO_REBOOT,
@@ -565,7 +567,8 @@ mymain(void)
QEMU_CAPS_DEVICE_QXL,
QEMU_CAPS_DEVICE_VGA,
QEMU_CAPS_DEVICE_CIRRUS_VGA,
- QEMU_CAPS_DEVICE_VMWARE_SVGA);
+ QEMU_CAPS_DEVICE_VMWARE_SVGA,
+ QEMU_CAPS_DEVICE_USB_SERIAL);
DO_TEST("qemu-kvm-0.12.1.2-rhel62-beta", 12001, 1, 0,
QEMU_CAPS_VNC_COLON,
QEMU_CAPS_NO_REBOOT,
@@ -715,7 +718,8 @@ mymain(void)
QEMU_CAPS_DEVICE_QXL,
QEMU_CAPS_DEVICE_VGA,
QEMU_CAPS_DEVICE_CIRRUS_VGA,
- QEMU_CAPS_DEVICE_VMWARE_SVGA);
+ QEMU_CAPS_DEVICE_VMWARE_SVGA,
+ QEMU_CAPS_DEVICE_USB_SERIAL);
DO_TEST("qemu-1.1.0", 1001000, 0, 0,
QEMU_CAPS_VNC_COLON,
QEMU_CAPS_NO_REBOOT,
@@ -801,7 +805,8 @@ mymain(void)
QEMU_CAPS_DEVICE_QXL,
QEMU_CAPS_DEVICE_VGA,
QEMU_CAPS_DEVICE_CIRRUS_VGA,
- QEMU_CAPS_DEVICE_VMWARE_SVGA);
+ QEMU_CAPS_DEVICE_VMWARE_SVGA,
+ QEMU_CAPS_DEVICE_USB_SERIAL);
DO_TEST("qemu-1.2.0", 1002000, 0, 0,
QEMU_CAPS_VNC_COLON,
QEMU_CAPS_NO_REBOOT,
@@ -898,7 +903,8 @@ mymain(void)
QEMU_CAPS_DEVICE_VGA,
QEMU_CAPS_DEVICE_CIRRUS_VGA,
QEMU_CAPS_DEVICE_VMWARE_SVGA,
- QEMU_CAPS_DEVICE_VIDEO_PRIMARY);
+ QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
+ QEMU_CAPS_DEVICE_USB_SERIAL);
DO_TEST("qemu-kvm-1.2.0", 1002000, 1, 0,
QEMU_CAPS_VNC_COLON,
QEMU_CAPS_NO_REBOOT,
@@ -1000,7 +1006,8 @@ mymain(void)
QEMU_CAPS_DEVICE_VGA,
QEMU_CAPS_DEVICE_CIRRUS_VGA,
QEMU_CAPS_DEVICE_VMWARE_SVGA,
- QEMU_CAPS_DEVICE_VIDEO_PRIMARY);
+ QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
+ QEMU_CAPS_DEVICE_USB_SERIAL);
return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
}
--
1.7.11.4
12:45 p.m.
New subject: [libvirt] [PATCH 1/2] qemu: add usb-serial caps flag
On 01/02/2013 11:57 PM, Guannan Ren wrote:
QEMU_CAPS_DEVICE_USB_SERIAL /* -device usb-serial */
---
src/qemu/qemu_capabilities.c | 2 ++
src/qemu/qemu_capabilities.h | 1 +
tests/qemuhelptest.c | 21 ++++++++++++++-------
3 files changed, 17 insertions(+), 7 deletions(-)
ACK.
DO_TEST("qemu-kvm-1.2.0", 1002000, 1, 0,
QEMU_CAPS_VNC_COLON,
QEMU_CAPS_NO_REBOOT,
@@ -1000,7 +1006,8 @@ mymain(void)
QEMU_CAPS_DEVICE_VGA,
QEMU_CAPS_DEVICE_CIRRUS_VGA,
QEMU_CAPS_DEVICE_VMWARE_SVGA,
- QEMU_CAPS_DEVICE_VIDEO_PRIMARY);
+ QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
+ QEMU_CAPS_DEVICE_USB_SERIAL);
Side question - why are we still parsing -help output of 1.2.0 in our
help test, since that should be using QMP instead? And when are we
going to improve our testsuite to make sure our QMP capability parsing
is doing what we want? But doesn't affect this patch.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
12:57 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: add usb-serial support
Add an optional 'type' attribute to <target> element of serial port
device. There are two choices for its value, 'isa-serial' and
'usb-serial'. For backward compatibility, when attribute 'type' is
missing the 'isa-serial' will be chose as before.
Libvirt XML sample
<serial type='pty'>
<target type='usb-serial' port='0'/>
<address type='usb' bus='0' port='1'/>
</serial>
qemu commandline:
qemu ${other_vm_args} \
-chardev pty,id=charserial0 \
-device usb-serial,chardev=charserial0,id=serial0,bus=usb.0,port=1
---
docs/formatdomain.html.in | 9 ++++++++-
docs/schemas/domaincommon.rng | 20 +++++++++++++++++---
src/conf/domain_conf.c | 42 +++++++++++++++++++++++++++++++++++++++---
src/conf/domain_conf.h | 10 ++++++++++
src/libvirt_private.syms | 2 ++
src/qemu/qemu_command.c | 24 ++++++++++++++++++++++--
6 files changed, 98 insertions(+), 9 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 94df6f8..7241bb9 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -3677,7 +3677,14 @@ qemu-kvm -net nic,model=? /dev/null
<p>
<code>target</code> can have a <code>port</code> attribute,
which
specifies the port number. Ports are numbered starting from 0. There are
- usually 0, 1 or 2 serial ports.
+ usually 0, 1 or 2 serial ports. There is also an optional
+ <code>type</code> attribute <span class="since">Since
1.0.2</span>
+ which has two choices for its value, one is< code>isa-serial</code>,
+ the other is <code>usb-serial</code>. If <code>type</code>
is missing,
+ <code>isa-serial</code> will be used by default. For
<code>usb-serial</code>
+ an optional sub-element <code><address></code> with
+ <code>type='usb'</code>which can tie the device to a particular
controller,
+ <a href="#elementsAddress">documentedabove</a>.
</p>
<h6><a name="elementCharConsole">Console</a></h6>
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 0529d62..7b7a2f9 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -2453,12 +2453,26 @@
</attribute>
</define>
+ <define name='qemucdevSerialTgtType'>
+ <attribute name='type'>
+ <choice>
+ <value>isa-serial</value>
+ <value>usb-serial</value>
+ </choice>
+ </attribute>
+ </define>
+
<define name="qemucdevTgtDef">
<element name="target">
<interleave>
- <optional>
- <ref name="qemucdevConsoleTgtType"/>
- </optional>
+ <choice>
+ <optional>
+ <ref name="qemucdevConsoleTgtType"/>
+ </optional>
+ <optional>
+ <ref name="qemucdevSerialTgtType"/>
+ </optional>
+ </choice>
<optional>
<attribute name="port"/>
</optional>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 79af087..9ece160 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -336,6 +336,11 @@ VIR_ENUM_IMPL(virDomainNetInterfaceLinkState,
VIR_DOMAIN_NET_INTERFACE_LINK_STAT
"up",
"down")
+VIR_ENUM_IMPL(virDomainChrSerialTarget,
+ VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_LAST,
+ "isa-serial",
+ "usb-serial")
+
VIR_ENUM_IMPL(virDomainChrChannelTarget,
VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_LAST,
"none",
@@ -5445,6 +5450,9 @@ virDomainChrDefaultTargetType(virCapsPtr caps,
break;
case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL:
+ target = VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA;
+ break;
+
case VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL:
default:
/* No target type yet*/
@@ -5457,7 +5465,8 @@ virDomainChrDefaultTargetType(virCapsPtr caps,
static int
virDomainChrTargetTypeFromString(virCapsPtr caps,
- virDomainDefPtr def,
+ virDomainDefPtr vmdef,
+ virDomainChrDefPtr def,
int devtype,
const char *targetType)
{
@@ -5465,7 +5474,7 @@ virDomainChrTargetTypeFromString(virCapsPtr caps,
int target = 0;
if (!targetType) {
- target = virDomainChrDefaultTargetType(caps, def, devtype);
+ target = virDomainChrDefaultTargetType(caps, vmdef, devtype);
goto out;
}
@@ -5479,12 +5488,17 @@ virDomainChrTargetTypeFromString(virCapsPtr caps,
break;
case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL:
+ target = virDomainChrSerialTargetTypeFromString(targetType);
+ break;
+
case VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL:
default:
/* No target type yet*/
break;
}
+ def->targetType_attr = true;
+
out:
ret = target;
return ret;
@@ -5503,7 +5517,7 @@ virDomainChrDefParseTargetXML(virCapsPtr caps,
const char *portStr = NULL;
if ((def->targetType =
- virDomainChrTargetTypeFromString(caps, vmdef,
+ virDomainChrTargetTypeFromString(caps, vmdef, def,
def->deviceType, targetType)) < 0) {
virReportError(VIR_ERR_XML_ERROR,
_("unknown target type '%s' specified for character
device"),
@@ -5936,6 +5950,15 @@ virDomainChrDefParseXML(virCapsPtr caps,
if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0)
goto error;
+ if (def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL &&
+ def->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB &&
+ def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
+ def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("usb-serial requires address of usb type"));
+ goto error;
+ }
+
cleanup:
VIR_FREE(type);
@@ -7939,6 +7962,9 @@ virDomainChrTargetTypeToString(int deviceType,
case VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE:
type = virDomainChrConsoleTargetTypeToString(targetType);
break;
+ case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL:
+ type = virDomainChrSerialTargetTypeToString(targetType);
+ break;
default:
break;
}
@@ -13088,6 +13114,15 @@ virDomainChrDefFormat(virBufferPtr buf,
def->target.port);
break;
+ case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL:
+ if (def->targetType_attr) {
+ virBufferAsprintf(buf,
+ " <target type='%s'
port='%d'/>\n",
+ virDomainChrTargetTypeToString(def->deviceType,
+ def->targetType),
+ def->target.port);
+ break;
+ }
default:
virBufferAsprintf(buf, " <target port='%d'/>\n",
def->target.port);
@@ -14415,6 +14450,7 @@ virDomainDefFormatInternal(virDomainDefPtr def,
(n < def->nserials)) {
memcpy(&console, def->serials[n], sizeof(console));
console.deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE;
+ console.targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL;
} else {
memcpy(&console, def->consoles[n], sizeof(console));
}
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index a975a63..0e2746b 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -909,6 +909,13 @@ enum virDomainChrDeviceType {
VIR_DOMAIN_CHR_DEVICE_TYPE_LAST
};
+enum virDomainChrSerialTargetType {
+ VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA = 0,
+ VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB,
+
+ VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_LAST
+};
+
enum virDomainChrChannelTargetType {
VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_NONE = 0,
VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD,
@@ -994,6 +1001,8 @@ struct _virDomainChrSourceDef {
/* A complete character device, both host and domain views. */
struct _virDomainChrDef {
int deviceType;
+
+ bool targetType_attr;
int targetType;
union {
int port; /* parallel, serial, console */
@@ -2250,6 +2259,7 @@ VIR_ENUM_DECL(virDomainNetInterfaceLinkState)
VIR_ENUM_DECL(virDomainChrDevice)
VIR_ENUM_DECL(virDomainChrChannelTarget)
VIR_ENUM_DECL(virDomainChrConsoleTarget)
+VIR_ENUM_DECL(virDomainChrSerialTarget)
VIR_ENUM_DECL(virDomainSmartcard)
VIR_ENUM_DECL(virDomainChr)
VIR_ENUM_DECL(virDomainChrTcpProtocol)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 497d5d3..aefdf9c 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -293,6 +293,8 @@ virDomainChrConsoleTargetTypeToString;
virDomainChrDefForeach;
virDomainChrDefFree;
virDomainChrDefNew;
+virDomainChrSerialTargetTypeFromString;
+virDomainChrSerialTargetTypeToString;
virDomainChrSourceDefCopy;
virDomainChrSourceDefFree;
virDomainChrSpicevmcTypeFromString;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 8a3de09..879b943 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6975,10 +6975,30 @@ qemuBuildChrDeviceStr(virDomainChrDefPtr serial,
if (qemuBuildDeviceAddressStr(&cmd, &serial->info, caps) < 0)
goto error;
}
- } else
- virBufferAsprintf(&cmd, "isa-serial,chardev=char%s,id=%s",
+ } else {
+ virBufferAsprintf(&cmd, "%s,chardev=char%s,id=%s",
+ virDomainChrSerialTargetTypeToString(serial->targetType),
serial->info.alias, serial->info.alias);
+ if (serial->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB) {
+ if (!qemuCapsGet(caps, QEMU_CAPS_DEVICE_USB_SERIAL)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("usb-serial is not supported in this QEMU
binary"));
+ goto error;
+ }
+
+ if (serial->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
+ serial->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("usb-serial requires address of usb type"));
+ goto error;
+ }
+
+ if (qemuBuildDeviceAddressStr(&cmd, &serial->info, caps) < 0)
+ goto error;
+ }
+ }
+
if (virBufferError(&cmd)) {
virReportOOMError();
goto error;
--
1.7.11.4
2:24 p.m.
New subject: [libvirt] [PATCH 2/2] qemu: add usb-serial support
On 01/02/2013 11:57 PM, Guannan Ren wrote:
Add an optional 'type' attribute to <target> element of
serial port
device. There are two choices for its value, 'isa-serial' and
'usb-serial'. For backward compatibility, when attribute 'type' is
missing the 'isa-serial' will be chose as before.
s/chose/chosen/
Libvirt XML sample
<serial type='pty'>
<target type='usb-serial' port='0'/>
<address type='usb' bus='0' port='1'/>
</serial>
qemu commandline:
qemu ${other_vm_args} \
-chardev pty,id=charserial0 \
-device usb-serial,chardev=charserial0,id=serial0,bus=usb.0,port=1
+++ b/docs/formatdomain.html.in
@@ -3677,7 +3677,14 @@ qemu-kvm -net nic,model=? /dev/null
<p>
<code>target</code> can have a <code>port</code>
attribute, which
specifies the port number. Ports are numbered starting from 0. There are
- usually 0, 1 or 2 serial ports.
+ usually 0, 1 or 2 serial ports. There is also an optional
+ <code>type</code> attribute <span class="since">Since
1.0.2</span>
This is mid-sentence, so s/Since/since/
+ which has two choices for its value, one is<
code>isa-serial</code>,
+ the other is <code>usb-serial</code>. If <code>type</code>
is missing,
+ <code>isa-serial</code> will be used by default. For
<code>usb-serial</code>
+ an optional sub-element <code><address></code> with
+ <code>type='usb'</code>which can tie the device to a
particular controller,
+ <a href="#elementsAddress">documentedabove</a>.
s/documentedabove/documented above/
@@ -994,6 +1001,8 @@ struct _virDomainChrSourceDef {
/* A complete character device, both host and domain views. */
struct _virDomainChrDef {
int deviceType;
+
+ bool targetType_attr;
The naming looks funny, when you are mixing camelCase and
underscore_split in the same name. Also, do we really need this
boolean? How many test files would have to change if we change all
existing outputs to always include type='isa-serial', where the field is
optional only on input; vs. your approach of using this bool to omit the
field on output if the user did not specify it on input?
Overall it looks reasonable, but depending on the answer to whether we
should just always output type='isa-serial', you may have a v2 to submit.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11:25 p.m.
New subject: [libvirt] [PATCH 2/2] qemu: add usb-serial support
On 01/05/2013 04:24 AM, Eric Blake wrote:
Overall it looks reasonable, but depending on the answer to whether
we
should just always output type='isa-serial', you may have a v2 to submit.
As attribute 'type' is optional, my initial thoughts is that If
type='value' is given
explicitly on input, then we print it out on output too, if no type
is given on input,
we don't print it out on output either. Backward compatibility and
being more flexible.
There are indeed a number of testcases we need to modify if we
always output
type='isa-serial', but not a big workload.
Guannan
5:01 p.m.
New subject: [libvirt] [PATCH 2/2] qemu: add usb-serial support
On 01/04/2013 10:25 PM, Guannan Ren wrote:
On 01/05/2013 04:24 AM, Eric Blake wrote:
> Overall it looks reasonable, but depending on the answer to whether we
> should just always output type='isa-serial', you may have a v2 to submit.
>
As attribute 'type' is optional, my initial thoughts is that If
type='value' is given
explicitly on input, then we print it out on output too, if no type
is given on input,
we don't print it out on output either. Backward compatibility and
being more flexible.
Backward compatibility says that we must never fail to parse an older
XML that used to be valid (therefore, the new attribute must be optional
on input, and have a sane default when omitted). But it does not
require that we cannot output more information than was provided on
input (for example, we compute and output <address> elements for many
devices, even when not present on input). Older code already has to be
prepared to ignore attributes and elements that get output by newer
libvirt. If having that information makes the output more legible, then
I think it is worth always outputting the information by default.
There are indeed a number of testcases we need to modify if we
always output
type='isa-serial', but not a big workload.
Then I think it's better to always output type='isa-serial' even when
the input omitted it. Anyone else with an opinion?
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11:25 p.m.
New subject: [libvirt] [PATCH v2 2/2] qemu: add usb-serial support
Add an optional 'type' attribute to <target> element of serial port
device. There are two choices for its value, 'isa-serial' and
'usb-serial'. For backward compatibility, when attribute 'type' is
missing the 'isa-serial' will be chosen as before.
Libvirt XML sample
<serial type='pty'>
<target type='usb-serial' port='0'/>
<address type='usb' bus='0' port='1'/>
</serial>
qemu commandline:
qemu ${other_vm_args} \
-chardev pty,id=charserial0 \
-device usb-serial,chardev=charserial0,id=serial0,bus=usb.0,port=1
---
docs/formatdomain.html.in | 9 ++++++++-
docs/schemas/domaincommon.rng | 20 +++++++++++++++++---
src/conf/domain_conf.c | 42 +++++++++++++++++++++++++++++++++++++++---
src/conf/domain_conf.h | 10 ++++++++++
src/libvirt_private.syms | 2 ++
src/qemu/qemu_command.c | 24 ++++++++++++++++++++++--
6 files changed, 98 insertions(+), 9 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 94df6f8..7811056 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -3677,7 +3677,14 @@ qemu-kvm -net nic,model=? /dev/null
<p>
<code>target</code> can have a <code>port</code> attribute,
which
specifies the port number. Ports are numbered starting from 0. There are
- usually 0, 1 or 2 serial ports.
+ usually 0, 1 or 2 serial ports. There is also an optional
+ <code>type</code> attribute <span class="since">since
1.0.2</span>
+ which has two choices for its value, one is< code>isa-serial</code>,
+ the other is <code>usb-serial</code>. If <code>type</code>
is missing,
+ <code>isa-serial</code> will be used by default. For
<code>usb-serial</code>
+ an optional sub-element <code><address></code> with
+ <code>type='usb'</code>which can tie the device to a particular
controller,
+ <a href="#elementsAddress">documented above</a>.
</p>
<h6><a name="elementCharConsole">Console</a></h6>
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 0529d62..7b7a2f9 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -2453,12 +2453,26 @@
</attribute>
</define>
+ <define name='qemucdevSerialTgtType'>
+ <attribute name='type'>
+ <choice>
+ <value>isa-serial</value>
+ <value>usb-serial</value>
+ </choice>
+ </attribute>
+ </define>
+
<define name="qemucdevTgtDef">
<element name="target">
<interleave>
- <optional>
- <ref name="qemucdevConsoleTgtType"/>
- </optional>
+ <choice>
+ <optional>
+ <ref name="qemucdevConsoleTgtType"/>
+ </optional>
+ <optional>
+ <ref name="qemucdevSerialTgtType"/>
+ </optional>
+ </choice>
<optional>
<attribute name="port"/>
</optional>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 79af087..ccc7931 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -336,6 +336,11 @@ VIR_ENUM_IMPL(virDomainNetInterfaceLinkState,
VIR_DOMAIN_NET_INTERFACE_LINK_STAT
"up",
"down")
+VIR_ENUM_IMPL(virDomainChrSerialTarget,
+ VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_LAST,
+ "isa-serial",
+ "usb-serial")
+
VIR_ENUM_IMPL(virDomainChrChannelTarget,
VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_LAST,
"none",
@@ -5445,6 +5450,9 @@ virDomainChrDefaultTargetType(virCapsPtr caps,
break;
case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL:
+ target = VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA;
+ break;
+
case VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL:
default:
/* No target type yet*/
@@ -5457,7 +5465,8 @@ virDomainChrDefaultTargetType(virCapsPtr caps,
static int
virDomainChrTargetTypeFromString(virCapsPtr caps,
- virDomainDefPtr def,
+ virDomainDefPtr vmdef,
+ virDomainChrDefPtr def,
int devtype,
const char *targetType)
{
@@ -5465,7 +5474,7 @@ virDomainChrTargetTypeFromString(virCapsPtr caps,
int target = 0;
if (!targetType) {
- target = virDomainChrDefaultTargetType(caps, def, devtype);
+ target = virDomainChrDefaultTargetType(caps, vmdef, devtype);
goto out;
}
@@ -5479,12 +5488,17 @@ virDomainChrTargetTypeFromString(virCapsPtr caps,
break;
case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL:
+ target = virDomainChrSerialTargetTypeFromString(targetType);
+ break;
+
case VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL:
default:
/* No target type yet*/
break;
}
+ def->targetTypeAttr = true;
+
out:
ret = target;
return ret;
@@ -5503,7 +5517,7 @@ virDomainChrDefParseTargetXML(virCapsPtr caps,
const char *portStr = NULL;
if ((def->targetType =
- virDomainChrTargetTypeFromString(caps, vmdef,
+ virDomainChrTargetTypeFromString(caps, vmdef, def,
def->deviceType, targetType)) < 0) {
virReportError(VIR_ERR_XML_ERROR,
_("unknown target type '%s' specified for character
device"),
@@ -5936,6 +5950,15 @@ virDomainChrDefParseXML(virCapsPtr caps,
if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0)
goto error;
+ if (def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL &&
+ def->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB &&
+ def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
+ def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("usb-serial requires address of usb type"));
+ goto error;
+ }
+
cleanup:
VIR_FREE(type);
@@ -7939,6 +7962,9 @@ virDomainChrTargetTypeToString(int deviceType,
case VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE:
type = virDomainChrConsoleTargetTypeToString(targetType);
break;
+ case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL:
+ type = virDomainChrSerialTargetTypeToString(targetType);
+ break;
default:
break;
}
@@ -13088,6 +13114,15 @@ virDomainChrDefFormat(virBufferPtr buf,
def->target.port);
break;
+ case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL:
+ if (def->targetTypeAttr) {
+ virBufferAsprintf(buf,
+ " <target type='%s'
port='%d'/>\n",
+ virDomainChrTargetTypeToString(def->deviceType,
+ def->targetType),
+ def->target.port);
+ break;
+ }
default:
virBufferAsprintf(buf, " <target port='%d'/>\n",
def->target.port);
@@ -14415,6 +14450,7 @@ virDomainDefFormatInternal(virDomainDefPtr def,
(n < def->nserials)) {
memcpy(&console, def->serials[n], sizeof(console));
console.deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE;
+ console.targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL;
} else {
memcpy(&console, def->consoles[n], sizeof(console));
}
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index a975a63..93d2265 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -909,6 +909,13 @@ enum virDomainChrDeviceType {
VIR_DOMAIN_CHR_DEVICE_TYPE_LAST
};
+enum virDomainChrSerialTargetType {
+ VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA = 0,
+ VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB,
+
+ VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_LAST
+};
+
enum virDomainChrChannelTargetType {
VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_NONE = 0,
VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD,
@@ -994,6 +1001,8 @@ struct _virDomainChrSourceDef {
/* A complete character device, both host and domain views. */
struct _virDomainChrDef {
int deviceType;
+
+ bool targetTypeAttr;
int targetType;
union {
int port; /* parallel, serial, console */
@@ -2250,6 +2259,7 @@ VIR_ENUM_DECL(virDomainNetInterfaceLinkState)
VIR_ENUM_DECL(virDomainChrDevice)
VIR_ENUM_DECL(virDomainChrChannelTarget)
VIR_ENUM_DECL(virDomainChrConsoleTarget)
+VIR_ENUM_DECL(virDomainChrSerialTarget)
VIR_ENUM_DECL(virDomainSmartcard)
VIR_ENUM_DECL(virDomainChr)
VIR_ENUM_DECL(virDomainChrTcpProtocol)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 5636661..6cd91d0 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -293,6 +293,8 @@ virDomainChrConsoleTargetTypeToString;
virDomainChrDefForeach;
virDomainChrDefFree;
virDomainChrDefNew;
+virDomainChrSerialTargetTypeFromString;
+virDomainChrSerialTargetTypeToString;
virDomainChrSourceDefCopy;
virDomainChrSourceDefFree;
virDomainChrSpicevmcTypeFromString;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 8a3de09..879b943 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6975,10 +6975,30 @@ qemuBuildChrDeviceStr(virDomainChrDefPtr serial,
if (qemuBuildDeviceAddressStr(&cmd, &serial->info, caps) < 0)
goto error;
}
- } else
- virBufferAsprintf(&cmd, "isa-serial,chardev=char%s,id=%s",
+ } else {
+ virBufferAsprintf(&cmd, "%s,chardev=char%s,id=%s",
+ virDomainChrSerialTargetTypeToString(serial->targetType),
serial->info.alias, serial->info.alias);
+ if (serial->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB) {
+ if (!qemuCapsGet(caps, QEMU_CAPS_DEVICE_USB_SERIAL)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("usb-serial is not supported in this QEMU
binary"));
+ goto error;
+ }
+
+ if (serial->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
+ serial->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("usb-serial requires address of usb type"));
+ goto error;
+ }
+
+ if (qemuBuildDeviceAddressStr(&cmd, &serial->info, caps) < 0)
+ goto error;
+ }
+ }
+
if (virBufferError(&cmd)) {
virReportOOMError();
goto error;
--
1.7.11.4
5:12 p.m.
New subject: [libvirt] [PATCH v2 2/2] qemu: add usb-serial support
On 01/04/2013 10:25 PM, Guannan Ren wrote:
Add an optional 'type' attribute to <target> element of
serial port
device. There are two choices for its value, 'isa-serial' and
'usb-serial'. For backward compatibility, when attribute 'type' is
missing the 'isa-serial' will be chosen as before.
Libvirt XML sample
<serial type='pty'>
<target type='usb-serial' port='0'/>
<address type='usb' bus='0' port='1'/>
</serial>
qemu commandline:
qemu ${other_vm_args} \
-chardev pty,id=charserial0 \
-device usb-serial,chardev=charserial0,id=serial0,bus=usb.0,port=1
+++ b/docs/formatdomain.html.in
@@ -3677,7 +3677,14 @@ qemu-kvm -net nic,model=? /dev/null
<p>
<code>target</code> can have a <code>port</code>
attribute, which
specifies the port number. Ports are numbered starting from 0. There are
- usually 0, 1 or 2 serial ports.
+ usually 0, 1 or 2 serial ports. There is also an optional
+ <code>type</code> attribute <span class="since">since
1.0.2</span>
+ which has two choices for its value, one is< code>isa-serial</code>,
+ the other is <code>usb-serial</code>. If <code>type</code>
is missing,
+ <code>isa-serial</code> will be used by default. For
<code>usb-serial</code>
+ an optional sub-element <code><address></code> with
+ <code>type='usb'</code>which can tie the device to a
particular controller,
This renders as:
sub-element <address> with type='usb'which can tie
You are missing a space, and it also sounds funny, so I suggest:
s/which / /
@@ -994,6 +1001,8 @@ struct _virDomainChrSourceDef {
/* A complete character device, both host and domain views. */
struct _virDomainChrDef {
int deviceType;
+
+ bool targetTypeAttr;
We still haven't answered the question of whether it is better to update
all tests to add an output of explicit type='isa-serial' even when
omitted on input (and this bool is not needed), or whether your approach
of doing output only when present on input makes more sense. If the
former, then we need a v3 that touches all affected tests; if the
latter, then ACK to this patch with the doc fix.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
7:45 a.m.
New subject: [libvirt] [PATCH v2 2/2] qemu: add usb-serial support
On 01/08/2013 07:12 AM, Eric Blake wrote:
This renders as:
sub-element <address> with type='usb'which can tie
You are missing a space, and it also sounds funny, so I suggest:
s/which / /
fixed.
> @@ -994,6 +1001,8 @@ struct _virDomainChrSourceDef {
> /* A complete character device, both host and domain views. */
> struct _virDomainChrDef {
> int deviceType;
> +
> + bool targetTypeAttr;
We still haven't answered the question of whether it is better to update
all tests to add an output of explicit type='isa-serial' even when
omitted on input (and this bool is not needed), or whether your approach
of doing output only when present on input makes more sense. If the
former, then we need a v3 that touches all affected tests; if the
latter, then ACK to this patch with the doc fix.
I prefer the latter without no third opinion.
Thanks for the review.
Pushed.
4335
days inactive
4342
days old
9 comments
2 participants
participants (2)
-
Eric Blake -
Guannan Ren