[libvirt] [PATCHv2 0/8] VirtIO RNG device support

Version 2 fixes comments by John and several improvements I noticed. There's 1 new patch in this series. See patch notes for more info. Peter Krempa (8): conf: Add fake switch statement to warn for new device types doc: schema: Add basic documentation for the virtual RNG device support conf: Add support for RNG device configuration in XML conf: Add RNG device ABI compatibility check qemu: Implement support for default 'random' backend for virtio-rng qemu: Implement support for EGD backend for virtio-rng tests: Add tests for virtio-rng device handling virtio-rng: Add rate limiting options for virtio-RNG docs/formatdomain.html.in | 78 ++++++ docs/schemas/domaincommon.rng | 48 ++++ src/conf/domain_conf.c | 293 ++++++++++++++++++++- src/conf/domain_conf.h | 39 +++ src/libvirt_private.syms | 2 + src/qemu/qemu_capabilities.c | 6 + src/qemu/qemu_capabilities.h | 4 + src/qemu/qemu_command.c | 123 +++++++++ .../qemuxml2argv-virtio-rng-egd.args | 1 + .../qemuxml2argv-virtio-rng-egd.xml | 26 ++ .../qemuxml2argv-virtio-rng-random.args | 1 + .../qemuxml2argv-virtio-rng-random.xml | 24 ++ tests/qemuxml2argvtest.c | 5 + tests/qemuxml2xmltest.c | 3 + 14 files changed, 652 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-egd.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-egd.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml -- 1.8.1.1

This patch adds a fake switch statement to force the compiler to warn after a new device type was added. This should remind the contributor to add the new device also to this iterator function. --- Notes: Version 2: - NEW in series, kind of RFC src/conf/domain_conf.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 10f361c..9e9fdb0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2348,6 +2348,35 @@ int virDomainDeviceInfoIterate(virDomainDefPtr def, if (cb(def, &device, &def->hubs[i]->info, opaque) < 0) return -1; } + + /* This switch statement is here to trigger compiler warning when adding + * a new device type. When you are adding a new field to the swtich you + * also have to add a iteration statement above. Otherwise the switch + * statement has no real function here and should be optimized out by the + * compiler. */ + i = VIR_DOMAIN_DEVICE_LAST; + switch ((virDomainDeviceType) i) { + case VIR_DOMAIN_DEVICE_DISK: + case VIR_DOMAIN_DEVICE_LEASE: + case VIR_DOMAIN_DEVICE_FS: + case VIR_DOMAIN_DEVICE_NET: + case VIR_DOMAIN_DEVICE_INPUT: + case VIR_DOMAIN_DEVICE_SOUND: + case VIR_DOMAIN_DEVICE_VIDEO: + case VIR_DOMAIN_DEVICE_HOSTDEV: + case VIR_DOMAIN_DEVICE_WATCHDOG: + case VIR_DOMAIN_DEVICE_CONTROLLER: + case VIR_DOMAIN_DEVICE_GRAPHICS: + case VIR_DOMAIN_DEVICE_HUB: + case VIR_DOMAIN_DEVICE_REDIRDEV: + case VIR_DOMAIN_DEVICE_NONE: + case VIR_DOMAIN_DEVICE_SMARTCARD: + case VIR_DOMAIN_DEVICE_CHR: + case VIR_DOMAIN_DEVICE_MEMBALLOON: + case VIR_DOMAIN_DEVICE_LAST: + break; + } + return 0; } -- 1.8.1.1

On 02/21/2013 07:47 AM, Peter Krempa wrote:
This patch adds a fake switch statement to force the compiler to warn after a new device type was added. This should remind the contributor to add the new device also to this iterator function. ---
Notes: Version 2: - NEW in series, kind of RFC
src/conf/domain_conf.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)
Interesting idea - but any time we can make the compiler help us be better maintainers, I'm in favor of it. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

This patch documents XML elements used for (basic) support of virtual RNG devices. In the devices section in the domain XML users may specify: For the default 'random' backend: <devices> <rng model='virtio'> <backend model='random'>/dev/urandom</backend> </rng> </devices> For the slightly more advanced EGD backend: <devices> <rng model='virtio'> <backend model='egd' type='udp'> <!-- this is a definition of a character device --> <source mode='bind' service='1234'/> <source mode='connect' host='1.2.3.4' service='1234'/> <!-- or other valid character device configuration --> </backend> </rng> </devices> For the planned random daemon/pool: <devices> <rng model='virtio'> <backend model='pool' pool='poolname'>class</backend> </devices> to enable the RNG device for guests. --- Notes: Version 2: - ACKed, no change, unfortunately doesn't make sense to push alone docs/formatdomain.html.in | 69 +++++++++++++++++++++++++++++++++++++++++++ docs/schemas/domaincommon.rng | 32 ++++++++++++++++++++ 2 files changed, 101 insertions(+) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index ffcc33e..e8cd086 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4278,6 +4278,75 @@ qemu-kvm -net nic,model=? /dev/null </ul> </dd> </dl> + <h4><a name="elementsRng">Random number generator device</a></h4> + + <p> + The virtual random number generator device allows the host to pass + through entropy to guest operating systems. + <span class="since">Since 1.0.3</span> + </p> + + <p> + Example: usage of the RNG device: + </p> +<pre> + ... + <devices> + <rng model='virtio'> + <backend model='random'>/dev/random</backend> + <!-- OR --> + <backend model='egd' type='udp'> + <source mode='bind' service='1234'> + <source mode='connect' host='1.2.3.4' service='1234'> + </backend> + </rng> + </devices> + ... +</pre> + <dl> + <dt><code>model</code></dt> + <dd> + <p> + The required <code>model</code> attribute specifies what type + of RNG device is provided. Valid values are specific to + the virtualization platform: + </p> + <ul> + <li>'virtio' — supported by qemu and virtio-rng kernel module</li> + </ul> + </dd> + <dt><code>backend</code></dt> + <dd> + <p> + The <code>backend</code> element specifies the source of entropy + to be used for the doimain. The source model is configured using the + <code>model</code> attribute. Supported source models are: + </p> + <ul> + <li>'random' — /dev/random (default) or similar device as source</li> + <li>'egd' — a EGD protocol backend. </li> + </ul> + </dd> + <dt><code>backend type='random'</code></dt> + <dd> + <p> + This backend type expects a non-blocking character device as input. + Examples of such devices are /dev/random and /dev/urandom. The file + name is specified as contents of the <code>backend</code> element. + When no file name is specified the hypervisor default is used. + </p> + </dd> + <dt><code>backend type='egd'</code></dt> + <dd> + <p> + This backend connects to a source using the EGD protocol. + The source is specified as a character device. Refer to + <a href='#elementsCharHostInterface'>character device host interface</a> + for more information. + </p> + </dd> + + </dl> <h3><a name="seclabel">Security label</a></h3> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 63be4aa..8330a50 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3126,6 +3126,7 @@ <ref name="hub"/> <ref name="redirdev"/> <ref name="redirfilter"/> + <ref name="rng"/> </choice> </zeroOrMore> <optional> @@ -3514,6 +3515,37 @@ </element> </define> + <define name="rng"> + <element name="rng"> + <attribute name="model"> + <choice> + <value>virtio</value> + </choice> + </attribute> + <ref name="rng-backend"/> + </element> + </define> + + <define name="rng-backend"> + <element name="backend"> + <choice> + <group> + <attribute name="model"> + <value>random</value> + </attribute> + <ref name="filePath"/> + </group> + <group> + <attribute name="model"> + <value>egd</value> + </attribute> + <ref name="qemucdevSrcType"/> + <ref name="qemucdevSrcDef"/> + </group> + </choice> + </element> + </define> + <define name="usbmaster"> <element name="master"> <attribute name="startport"> -- 1.8.1.1

On 02/21/2013 07:47 AM, Peter Krempa wrote:
This patch documents XML elements used for (basic) support of virtual RNG devices.
In the devices section in the domain XML users may specify:
For the default 'random' backend: <devices> <rng model='virtio'> <backend model='random'>/dev/urandom</backend> </rng> </devices>
For the slightly more advanced EGD backend: <devices> <rng model='virtio'> <backend model='egd' type='udp'> <!-- this is a definition of a character device --> <source mode='bind' service='1234'/> <source mode='connect' host='1.2.3.4' service='1234'/> <!-- or other valid character device configuration -->
You don't really allow two <source>; maybe a better layout would be a strategic comment, such as: <backend model='egd' type='udp'> <!-- this is a definition of a character device --> <source mode='bind' service='1234'/> <!-- or other valid character device configuration, such as <source mode='connect' host='1.2.3.4' service='1234'/> -->
</backend> </rng> </devices>
For the planned random daemon/pool: <devices> <rng model='virtio'> <backend model='pool' pool='poolname'>class</backend>
Missing </rng>
</devices>
to enable the RNG device for guests. ---
Notes: Version 2: - ACKed, no change, unfortunately doesn't make sense to push alone
Still some nits to fix before pushing:
+<pre> + ... + <devices> + <rng model='virtio'> + <backend model='random'>/dev/random</backend> + <!-- OR --> + <backend model='egd' type='udp'> + <source mode='bind' service='1234'> + <source mode='connect' host='1.2.3.4' service='1234'> + </backend> + </rng> + </devices> + ...
Do we really want two <source> in a single <backend> in the example, or would it be easier to show multiple <rng> devices, one for each type of backend?
+ <dd> + <p> + The <code>backend</code> element specifies the source of entropy + to be used for the doimain. The source model is configured using the
s/doimain/domain/
+ <code>model</code> attribute. Supported source models are: + </p> + <ul> + <li>'random' — /dev/random (default) or similar device as source</li> + <li>'egd' — a EGD protocol backend. </li>
not consistent on whether your <li> end with '.' -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 02/22/13 19:20, Eric Blake wrote:
On 02/21/2013 07:47 AM, Peter Krempa wrote:
This patch documents XML elements used for (basic) support of virtual RNG devices.
In the devices section in the domain XML users may specify:
For the default 'random' backend: <devices> <rng model='virtio'> <backend model='random'>/dev/urandom</backend> </rng> </devices>
For the slightly more advanced EGD backend: <devices> <rng model='virtio'> <backend model='egd' type='udp'> <!-- this is a definition of a character device --> <source mode='bind' service='1234'/> <source mode='connect' host='1.2.3.4' service='1234'/> <!-- or other valid character device configuration -->
You don't really allow two <source>; maybe a better layout would be a strategic comment, such as:
<backend model='egd' type='udp'> <!-- this is a definition of a character device --> <source mode='bind' service='1234'/> <!-- or other valid character device configuration, such as <source mode='connect' host='1.2.3.4' service='1234'/> -->
</backend> </rng> </devices>
For the planned random daemon/pool: <devices> <rng model='virtio'> <backend model='pool' pool='poolname'>class</backend>
Missing </rng>
</devices>
to enable the RNG device for guests. ---
Notes: Version 2: - ACKed, no change, unfortunately doesn't make sense to push alone
Still some nits to fix before pushing:
+<pre> + ... + <devices> + <rng model='virtio'> + <backend model='random'>/dev/random</backend> + <!-- OR --> + <backend model='egd' type='udp'> + <source mode='bind' service='1234'> + <source mode='connect' host='1.2.3.4' service='1234'> + </backend> + </rng> + </devices> + ...
Do we really want two <source> in a single <backend> in the example, or would it be easier to show multiple <rng> devices, one for each type of backend?
That actually is valid for the character device backends. The UDP backend has to use two separate sources for bi-directional communication. The definition of that source type is declared as a type in our RNG schema an I merely reused that.
+ <dd> + <p> + The <code>backend</code> element specifies the source of entropy + to be used for the doimain. The source model is configured using the
s/doimain/domain/
+ <code>model</code> attribute. Supported source models are: + </p> + <ul> + <li>'random' — /dev/random (default) or similar device as source</li> + <li>'egd' — a EGD protocol backend. </li>
not consistent on whether your <li> end with '.'
Peter

On 02/25/2013 02:08 AM, Peter Krempa wrote:
On 02/22/13 19:20, Eric Blake wrote:
On 02/21/2013 07:47 AM, Peter Krempa wrote:
This patch documents XML elements used for (basic) support of virtual RNG devices.
For the slightly more advanced EGD backend: <devices> <rng model='virtio'> <backend model='egd' type='udp'> <!-- this is a definition of a character device --> <source mode='bind' service='1234'/> <source mode='connect' host='1.2.3.4' service='1234'/> <!-- or other valid character device configuration -->
Do we really want two <source> in a single <backend> in the example, or would it be easier to show multiple <rng> devices, one for each type of backend?
That actually is valid for the character device backends. The UDP backend has to use two separate sources for bi-directional communication. The definition of that source type is declared as a type in our RNG schema an I merely reused that.
Oh, I didn't realize that. Do we want to ENFORCE that there are two <source> elements present then? Or can we always provide a sane default when the user provides only 0 or 1 source? Depending on that answer, it might be worth additional documentation on why two <source> elements are present, as well as mentioning that order is significant in how they are listed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 02/25/13 22:01, Eric Blake wrote:
On 02/25/2013 02:08 AM, Peter Krempa wrote:
On 02/22/13 19:20, Eric Blake wrote:
On 02/21/2013 07:47 AM, Peter Krempa wrote:
This patch documents XML elements used for (basic) support of virtual RNG devices.
For the slightly more advanced EGD backend: <devices> <rng model='virtio'> <backend model='egd' type='udp'> <!-- this is a definition of a character device --> <source mode='bind' service='1234'/> <source mode='connect' host='1.2.3.4' service='1234'/> <!-- or other valid character device configuration -->
Do we really want two <source> in a single <backend> in the example, or would it be easier to show multiple <rng> devices, one for each type of backend?
That actually is valid for the character device backends. The UDP backend has to use two separate sources for bi-directional communication. The definition of that source type is declared as a type in our RNG schema an I merely reused that.
Oh, I didn't realize that. Do we want to ENFORCE that there are two <source> elements present then? Or can we always provide a sane default when the user provides only 0 or 1 source? Depending on that answer, it might be worth additional documentation on why two <source> elements are present, as well as mentioning that order is significant in how they are listed.
I reused the code to facilitate the operations done with character devices. There's present infrastructure [1] that allows to parse, format and create commandlines for qemu. The defaults for the syntax of the XML and the expected values should be documented in the section describing character devices. (But I did not really check if the description is exhaustive) In hindsight I should have linked that section in the description of the egd backend model explicitly rather than by just mentioning it. Peter [1]: virDomainChrSourceDefParseXML, virDomainChrSourceDefFormat, qemuBuildChrChardevStr

On 02/25/13 22:20, Peter Krempa wrote:
On 02/25/13 22:01, Eric Blake wrote:
On 02/25/2013 02:08 AM, Peter Krempa wrote:
On 02/22/13 19:20, Eric Blake wrote:
On 02/21/2013 07:47 AM, Peter Krempa wrote:
This patch documents XML elements used for (basic) support of virtual RNG devices.
For the slightly more advanced EGD backend: <devices> <rng model='virtio'> <backend model='egd' type='udp'> <!-- this is a definition of a character device --> <source mode='bind' service='1234'/> <source mode='connect' host='1.2.3.4' service='1234'/> <!-- or other valid character device configuration -->
Do we really want two <source> in a single <backend> in the example, or would it be easier to show multiple <rng> devices, one for each type of backend?
That actually is valid for the character device backends. The UDP backend has to use two separate sources for bi-directional communication. The definition of that source type is declared as a type in our RNG schema an I merely reused that.
Oh, I didn't realize that. Do we want to ENFORCE that there are two <source> elements present then? Or can we always provide a sane default when the user provides only 0 or 1 source? Depending on that answer, it might be worth additional documentation on why two <source> elements are present, as well as mentioning that order is significant in how they are listed.
I reused the code to facilitate the operations done with character devices. There's present infrastructure [1] that allows to parse, format and create commandlines for qemu. The defaults for the syntax of the XML and the expected values should be documented in the section describing character devices. (But I did not really check if the description is exhaustive)
In hindsight I should have linked that section in the description of the egd backend model explicitly rather than by just mentioning it.
I actually did that: backend type='egd' (Hm there's a mistake here - s/type/model/, I'll fix that) This backend connects to a source using the EGD protocol. The source is specified as a character device. Refer to character device host interface for more information. (where "character device host interface" links to : http://libvirt.org/formatdomain.html#elementsCharHostInterface )
Peter
[1]: virDomainChrSourceDefParseXML, virDomainChrSourceDefFormat, qemuBuildChrChardevStr
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

This patch adds basic configuration support for the RNG device suporting the virtio model with the "random" and "egd" backend types as described in the schema in the previous patch. --- Notes: Version 2: - fix a ton of memory leaks (I assumed that virXMLGetProp returns static strings) - Add new device type to even more places - Fix error message c&p error - Fix memleak in RNGDef free func src/conf/domain_conf.c | 214 ++++++++++++++++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 37 ++++++++ src/libvirt_private.syms | 2 + 3 files changed, 252 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9e9fdb0..082f1f5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -175,7 +175,8 @@ VIR_ENUM_IMPL(virDomainDevice, VIR_DOMAIN_DEVICE_LAST, "redirdev", "smartcard", "chr", - "memballoon") + "memballoon", + "rng") VIR_ENUM_IMPL(virDomainDeviceAddress, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST, "none", @@ -700,6 +701,15 @@ VIR_ENUM_IMPL(virDomainNumatuneMemPlacementMode, "static", "auto"); +VIR_ENUM_IMPL(virDomainRNGModel, + VIR_DOMAIN_RNG_MODEL_LAST, + "virtio"); + +VIR_ENUM_IMPL(virDomainRNGBackend, + VIR_DOMAIN_RNG_BACKEND_LAST, + "random", + "egd"); + #define VIR_DOMAIN_XML_WRITE_FLAGS VIR_DOMAIN_XML_SECURE #define VIR_DOMAIN_XML_READ_FLAGS VIR_DOMAIN_XML_INACTIVE @@ -1597,6 +1607,9 @@ void virDomainDeviceDefFree(virDomainDeviceDefPtr def) case VIR_DOMAIN_DEVICE_REDIRDEV: virDomainRedirdevDefFree(def->data.redirdev); break; + case VIR_DOMAIN_DEVICE_RNG: + virDomainRNGDefFree(def->data.rng); + break; case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_FS: case VIR_DOMAIN_DEVICE_SMARTCARD: @@ -2342,6 +2355,12 @@ int virDomainDeviceInfoIterate(virDomainDefPtr def, if (cb(def, &device, &def->memballoon->info, opaque) < 0) return -1; } + if (def->rng) { + device.type = VIR_DOMAIN_DEVICE_RNG; + device.data.rng = def->rng; + if (cb(def, &device, &def->rng->info, opaque) < 0) + return -1; + } device.type = VIR_DOMAIN_DEVICE_HUB; for (i = 0; i < def->nhubs ; i++) { device.data.hub = def->hubs[i]; @@ -2374,6 +2393,7 @@ int virDomainDeviceInfoIterate(virDomainDefPtr def, case VIR_DOMAIN_DEVICE_CHR: case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_LAST: + case VIR_DOMAIN_DEVICE_RNG: break; } @@ -7452,6 +7472,115 @@ error: } +static virDomainRNGDefPtr +virDomainRNGDefParseXML(const xmlNodePtr node, + xmlXPathContextPtr ctxt, + unsigned int flags) +{ + const char *model = NULL; + const char *backend = NULL; + virDomainRNGDefPtr def; + xmlNodePtr save = ctxt->node; + xmlNodePtr *backends = NULL; + int nbackends; + + if (VIR_ALLOC(def) < 0) { + virReportOOMError(); + return NULL; + } + + if (!(model = virXMLPropString(node, "model"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", _("missing RNG device model")); + goto error; + } + + if ((def->model = virDomainRNGModelTypeFromString(model)) < 0) { + virReportError(VIR_ERR_XML_ERROR, _("unknown RNG model '%s'"), model); + goto error; + } + + ctxt->node = node; + + if ((nbackends = virXPathNodeSet("./backend", ctxt, &backends)) < 0) + goto error; + + if (nbackends != 1) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("only one RNG backend is supported")); + goto error; + } + + if (!(backend = virXMLPropString(backends[0], "model"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing RNG device backend model")); + goto error; + } + + if ((def->backend = virDomainRNGBackendTypeFromString(backend)) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown RNG backend model '%s'"), backend); + goto error; + } + + switch ((enum virDomainRNGBackend) def->backend) { + case VIR_DOMAIN_RNG_BACKEND_RANDOM: + def->source.file = virXPathString("string(./backend)", ctxt); + break; + + case VIR_DOMAIN_RNG_BACKEND_EGD: + { + char *type = virXMLPropString(backends[0], "type"); + if (!type) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing EGD backend type")); + goto error; + } + + + if (VIR_ALLOC(def->source.chardev) < 0) { + virReportOOMError(); + goto error; + } + + def->source.chardev->type = virDomainChrTypeFromString(type); + if (def->source.chardev->type < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown backend type '%s' for egd"), + type); + VIR_FREE(type); + goto error; + } + + VIR_FREE(type); + + if (virDomainChrSourceDefParseXML(def->source.chardev, + backends[0]->children, flags, + NULL, ctxt, NULL, 0) < 0) + goto error; + } + break; + + case VIR_DOMAIN_RNG_BACKEND_LAST: + break; + } + + if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) + goto error; + +cleanup: + VIR_FREE(model); + VIR_FREE(backend); + VIR_FREE(backends); + ctxt->node = save; + return def; + +error: + virDomainRNGDefFree(def); + def = NULL; + goto cleanup; +} + + static virDomainMemballoonDefPtr virDomainMemballoonDefParseXML(const xmlNodePtr node, unsigned int flags) @@ -8247,6 +8376,10 @@ virDomainDeviceDefParse(virCapsPtr caps, dev->type = VIR_DOMAIN_DEVICE_REDIRDEV; if (!(dev->data.redirdev = virDomainRedirdevDefParseXML(node, NULL, flags))) goto error; + } else if (xmlStrEqual(node->name, BAD_CAST "rng")) { + dev->type = VIR_DOMAIN_DEVICE_RNG; + if (!(dev->data.rng = virDomainRNGDefParseXML(node, ctxt, flags))) + goto error; } else { virReportError(VIR_ERR_XML_ERROR, "%s", _("unknown device type")); @@ -10601,6 +10734,22 @@ virDomainDefParseXML(virCapsPtr caps, } } + /* Parse the RNG device */ + if ((n = virXPathNodeSet("./devices/rng", ctxt, &nodes)) < 0) + goto error; + + if (n > 1) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("only a single RNG device is supported")); + goto error; + } + + if (n > 0) { + if (!(def->rng = virDomainRNGDefParseXML(nodes[0], ctxt, flags))) + goto error; + VIR_FREE(nodes); + } + /* analysis of the hub devices */ if ((n = virXPathNodeSet("./devices/hub", ctxt, &nodes)) < 0) { goto error; @@ -13623,6 +13772,63 @@ virDomainWatchdogDefFormat(virBufferPtr buf, } +static int +virDomainRNGDefFormat(virBufferPtr buf, + virDomainRNGDefPtr def, + unsigned int flags) +{ + const char *model = virDomainRNGModelTypeToString(def->model); + const char *backend = virDomainRNGBackendTypeToString(def->backend); + + virBufferAsprintf(buf, " <rng model='%s'>\n", model); + virBufferAsprintf(buf, " <backend model='%s'", backend); + + switch ((enum virDomainRNGBackend) def->backend) { + case VIR_DOMAIN_RNG_BACKEND_RANDOM: + if (def->source.file) + virBufferAsprintf(buf, ">%s</backend>\n", def->source.file); + else + virBufferAddLit(buf, "/>\n"); + + break; + + case VIR_DOMAIN_RNG_BACKEND_EGD: + virBufferAdjustIndent(buf, 2); + if (virDomainChrSourceDefFormat(buf, def->source.chardev, + false, flags) < 0) + return -1; + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, " </backend>\n"); + + case VIR_DOMAIN_RNG_BACKEND_LAST: + break; + } + + virBufferAddLit(buf, " </rng>\n"); + + return 0; +} + +void +virDomainRNGDefFree(virDomainRNGDefPtr def) +{ + if (!def) + return; + + switch ((enum virDomainRNGBackend) def->backend) { + case VIR_DOMAIN_RNG_BACKEND_RANDOM: + VIR_FREE(def->source.file); + break; + case VIR_DOMAIN_RNG_BACKEND_EGD: + virDomainChrSourceDefFree(def->source.chardev); + break; + case VIR_DOMAIN_RNG_BACKEND_LAST: + break; + } + + VIR_FREE(def); +} + static void virDomainVideoAccelDefFormat(virBufferPtr buf, virDomainVideoAccelDefPtr def) @@ -14816,6 +15022,9 @@ virDomainDefFormatInternal(virDomainDefPtr def, if (def->memballoon) virDomainMemballoonDefFormat(buf, def->memballoon, flags); + if (def->rng) + virDomainRNGDefFormat(buf, def->rng, flags); + virBufferAddLit(buf, " </devices>\n"); virBufferAdjustIndent(buf, 2); @@ -16086,6 +16295,9 @@ virDomainDeviceDefCopy(virCapsPtr caps, case VIR_DOMAIN_DEVICE_REDIRDEV: rc = virDomainRedirdevDefFormat(&buf, src->data.redirdev, flags); break; + case VIR_DOMAIN_DEVICE_RNG: + rc = virDomainRNGDefFormat(&buf, src->data.rng, flags); + break; case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_SMARTCARD: case VIR_DOMAIN_DEVICE_CHR: diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 4ffa4aa..0828954 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -114,6 +114,9 @@ typedef virDomainSnapshotObj *virDomainSnapshotObjPtr; typedef struct _virDomainSnapshotObjList virDomainSnapshotObjList; typedef virDomainSnapshotObjList *virDomainSnapshotObjListPtr; +typedef struct _virDomainRNGDef virDomainRNGDef; +typedef virDomainRNGDef *virDomainRNGDefPtr; + /* Flags for the 'type' field in virDomainDeviceDef */ typedef enum { VIR_DOMAIN_DEVICE_NONE = 0, @@ -133,6 +136,7 @@ typedef enum { VIR_DOMAIN_DEVICE_SMARTCARD, VIR_DOMAIN_DEVICE_CHR, VIR_DOMAIN_DEVICE_MEMBALLOON, + VIR_DOMAIN_DEVICE_RNG, VIR_DOMAIN_DEVICE_LAST } virDomainDeviceType; @@ -158,6 +162,7 @@ struct _virDomainDeviceDef { virDomainSmartcardDefPtr smartcard; virDomainChrDefPtr chr; virDomainMemballoonDefPtr memballoon; + virDomainRNGDefPtr rng; } data; }; @@ -1714,6 +1719,33 @@ struct _virBlkioDeviceWeight { unsigned int weight; }; +enum virDomainRNGModel { + VIR_DOMAIN_RNG_MODEL_VIRTIO, + + VIR_DOMAIN_RNG_MODEL_LAST +}; + +enum virDomainRNGBackend { + VIR_DOMAIN_RNG_BACKEND_RANDOM, + VIR_DOMAIN_RNG_BACKEND_EGD, + /* VIR_DOMAIN_RNG_BACKEND_POOL, */ + + VIR_DOMAIN_RNG_BACKEND_LAST +}; + +struct _virDomainRNGDef { + int model; + int backend; + + union { + char *file; /* file name for 'random' source */ + virDomainChrSourceDefPtr chardev; /* a char backend for + the EGD source */ + } source; + + virDomainDeviceInfo info; +}; + void virBlkioDeviceWeightArrayClear(virBlkioDeviceWeightPtr deviceWeights, int ndevices); @@ -1852,6 +1884,7 @@ struct _virDomainDef { virCPUDefPtr cpu; virSysinfoDefPtr sysinfo; virDomainRedirFilterDefPtr redirfilter; + virDomainRNGDefPtr rng; void *namespaceData; virDomainXMLNamespace ns; @@ -2065,6 +2098,8 @@ int virDomainEmulatorPinAdd(virDomainDefPtr def, int virDomainEmulatorPinDel(virDomainDefPtr def); +void virDomainRNGDefFree(virDomainRNGDefPtr def); + int virDomainDiskIndexByName(virDomainDefPtr def, const char *name, bool allow_ambiguous); const char *virDomainDiskPathByName(virDomainDefPtr, const char *name); @@ -2325,6 +2360,8 @@ VIR_ENUM_DECL(virDomainGraphicsSpiceMouseMode) VIR_ENUM_DECL(virDomainNumatuneMemMode) VIR_ENUM_DECL(virDomainNumatuneMemPlacementMode) VIR_ENUM_DECL(virDomainHyperv) +VIR_ENUM_DECL(virDomainRNGModel) +VIR_ENUM_DECL(virDomainRNGBackend) /* from libvirt.h */ VIR_ENUM_DECL(virDomainState) VIR_ENUM_DECL(virDomainNostateReason) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f399871..aed007e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -286,6 +286,8 @@ virDomainPMSuspendedReasonTypeFromString; virDomainPMSuspendedReasonTypeToString; virDomainRedirdevBusTypeFromString; virDomainRedirdevBusTypeToString; +virDomainRNGBackendTypeToString; +virDomainRNGModelTypeToString; virDomainRunningReasonTypeFromString; virDomainRunningReasonTypeToString; virDomainSaveConfig; -- 1.8.1.1

On 02/21/2013 07:47 AM, Peter Krempa wrote:
This patch adds basic configuration support for the RNG device suporting
s/suporting/supporting/
the virtio model with the "random" and "egd" backend types as described in the schema in the previous patch. ---
Notes: Version 2: - fix a ton of memory leaks (I assumed that virXMLGetProp returns static strings) - Add new device type to even more places - Fix error message c&p error - Fix memleak in RNGDef free func
+ case VIR_DOMAIN_RNG_BACKEND_EGD: + { + char *type = virXMLPropString(backends[0], "type"); + if (!type) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing EGD backend type")); + goto error; + } + + + if (VIR_ALLOC(def->source.chardev) < 0) { + virReportOOMError(); + goto error; + }
Memleak on 'type'.
@@ -10601,6 +10734,22 @@ virDomainDefParseXML(virCapsPtr caps, } }
+ /* Parse the RNG device */ + if ((n = virXPathNodeSet("./devices/rng", ctxt, &nodes)) < 0) + goto error; + + if (n > 1) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("only a single RNG device is supported"));
Is this an inherent limit of qemu? For that matter, is it an inherent limit, and no hypervisor can ever support more than one? In the bare metal case, can't you plug in multiple rng hardware dongles?
+static int +virDomainRNGDefFormat(virBufferPtr buf, + virDomainRNGDefPtr def, + unsigned int flags) +{ + const char *model = virDomainRNGModelTypeToString(def->model); + const char *backend = virDomainRNGBackendTypeToString(def->backend); + + virBufferAsprintf(buf, " <rng model='%s'>\n", model); + virBufferAsprintf(buf, " <backend model='%s'", backend); + + switch ((enum virDomainRNGBackend) def->backend) { + case VIR_DOMAIN_RNG_BACKEND_RANDOM: + if (def->source.file) + virBufferAsprintf(buf, ">%s</backend>\n", def->source.file);
Must use virBufferEscape(), as a random file name could contain characters that are special to XML. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 02/23/13 00:33, Eric Blake wrote:
On 02/21/2013 07:47 AM, Peter Krempa wrote:
This patch adds basic configuration support for the RNG device suporting
s/suporting/supporting/
the virtio model with the "random" and "egd" backend types as described in the schema in the previous patch. ---
Notes: Version 2: - fix a ton of memory leaks (I assumed that virXMLGetProp returns static strings) - Add new device type to even more places - Fix error message c&p error - Fix memleak in RNGDef free func
...
@@ -10601,6 +10734,22 @@ virDomainDefParseXML(virCapsPtr caps, } }
+ /* Parse the RNG device */ + if ((n = virXPathNodeSet("./devices/rng", ctxt, &nodes)) < 0) + goto error; + + if (n > 1) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("only a single RNG device is supported"));
Is this an inherent limit of qemu? For that matter, is it an inherent limit, and no hypervisor can ever support more than one? In the bare metal case, can't you plug in multiple rng hardware dongles?
AFAIK qemu is able to support multiple RNG devices but I didn't find that particularly useful. I will follow up with a patch that will add support for more than one RNG device, although I don't think it will be used much. Peter

On 02/25/13 10:42, Peter Krempa wrote:
On 02/23/13 00:33, Eric Blake wrote:
On 02/21/2013 07:47 AM, Peter Krempa wrote:
This patch adds basic configuration support for the RNG device suporting
s/suporting/supporting/
the virtio model with the "random" and "egd" backend types as described in the schema in the previous patch. ---
Notes: Version 2: - fix a ton of memory leaks (I assumed that virXMLGetProp returns static strings) - Add new device type to even more places - Fix error message c&p error - Fix memleak in RNGDef free func
...
@@ -10601,6 +10734,22 @@ virDomainDefParseXML(virCapsPtr caps, } }
+ /* Parse the RNG device */ + if ((n = virXPathNodeSet("./devices/rng", ctxt, &nodes)) < 0) + goto error; + + if (n > 1) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("only a single RNG device is supported"));
Is this an inherent limit of qemu? For that matter, is it an inherent limit, and no hypervisor can ever support more than one? In the bare metal case, can't you plug in multiple rng hardware dongles?
AFAIK qemu is able to support multiple RNG devices but I didn't find that particularly useful. I will follow up with a patch that will add support for more than one RNG device, although I don't think it will be used much.
I hacked up the patch to add support for multiple RNG's. qemu appears to support the device just fine. As of guest support it seems that it triggers a 100% reproducible kernel panic on linux guest when attempting to read from a machine with two virtio-rng devices. I filed a bug to track this issue: https://bugzilla.redhat.com/show_bug.cgi?id=915335 Peter

--- Notes: Version 2: - ACKed, no change, doesn't make sense to push alone src/conf/domain_conf.c | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 082f1f5..b6de57c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11700,6 +11700,36 @@ virDomainMemballoonDefCheckABIStability(virDomainMemballoonDefPtr src, static bool +virDomainRNGDefCheckABIStability(virDomainRNGDefPtr src, + virDomainRNGDefPtr dst) +{ + if (!src && !dst) + return true; + + if (!src || !dst) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target domain RNG device count '%d' " + "does not match source count '%d'"), + src ? 1 : 0, dst ? 1 : 0); + return false; + } + + if (src->model != dst->model) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target RNG model '%s' does not match source '%s'"), + virDomainRNGModelTypeToString(dst->model), + virDomainRNGModelTypeToString(src->model)); + return false; + } + + if (!virDomainDeviceInfoCheckABIStability(&src->info, &dst->info)) + return false; + + return true; +} + + +static bool virDomainHubDefCheckABIStability(virDomainHubDefPtr src, virDomainHubDefPtr dst) { @@ -12100,6 +12130,9 @@ virDomainDefCheckABIStability(virDomainDefPtr src, dst->memballoon)) return false; + if (!virDomainRNGDefCheckABIStability(src->rng, dst->rng)) + return false; + return true; } -- 1.8.1.1

On 02/21/2013 07:47 AM, Peter Krempa wrote:
---
Notes: Version 2: - ACKed, no change, doesn't make sense to push alone
Still good to go. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

This patch implements support for the virtio-rng-pci device and the rng-random backend in qemu. Two capabilities bits are added to track support for those: QEMU_CAPS_DEVICE_VIRTIO_RNG - for the device support and QEMU_CAPS_OBJECT_RNG_RANDOM - for the backend support. qemu is invoked with these additional parameters if the device is enabled: -object rng-random,id=rng0,filename=/test/phile (to add the backend) -device virtio-rng-pci,rng=rng0,bus=pci.0,addr=0x4 (to add the device) --- Notes: Version 2: - don't use different error messages for similar issues src/qemu/qemu_capabilities.c | 5 ++- src/qemu/qemu_capabilities.h | 3 ++ src/qemu/qemu_command.c | 98 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 105 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index af52bbf..a3d9e06 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -205,7 +205,8 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "usb-serial", /* 125 */ "usb-net", "add-fd", - + "virtio-rng", + "rng-random", ); struct _virQEMUCaps { @@ -1329,6 +1330,8 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { { "vmware-svga", QEMU_CAPS_DEVICE_VMWARE_SVGA }, { "usb-serial", QEMU_CAPS_DEVICE_USB_SERIAL}, { "usb-net", QEMU_CAPS_DEVICE_USB_NET}, + { "virtio-rng-pci", QEMU_CAPS_DEVICE_VIRTIO_RNG }, + { "rng-random", QEMU_CAPS_OBJECT_RNG_RANDOM }, }; diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index e69d558..ee0d0ca 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -166,6 +166,9 @@ enum virQEMUCapsFlags { QEMU_CAPS_DEVICE_USB_SERIAL = 125, /* -device usb-serial */ QEMU_CAPS_DEVICE_USB_NET = 126, /* -device usb-net */ QEMU_CAPS_ADD_FD = 127, /* -add-fd */ + QEMU_CAPS_DEVICE_VIRTIO_RNG = 128, /* virtio-rng device */ + QEMU_CAPS_OBJECT_RNG_RANDOM = 129, /* the rng-random backend for + virtio rng */ QEMU_CAPS_LAST, /* this must always be the last item */ }; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index dee493f..4a51413 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -787,6 +787,10 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) if (virAsprintf(&def->memballoon->info.alias, "balloon%d", 0) < 0) goto no_memory; } + if (def->rng) { + if (virAsprintf(&def->rng->info.alias, "rng%d", 0) < 0) + goto no_memory; + } return 0; @@ -1660,6 +1664,14 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, goto error; } + /* VirtIO RNG */ + if (def->rng && + def->rng->model == VIR_DOMAIN_RNG_MODEL_VIRTIO && + def->rng->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + if (qemuDomainPCIAddressSetNextAddr(addrs, &def->rng->info) < 0) + goto error; + } + /* A watchdog - skip IB700, it is not a PCI device */ if (def->watchdog && def->watchdog->model != VIR_DOMAIN_WATCHDOG_MODEL_IB700 && @@ -4165,6 +4177,82 @@ error: return NULL; } + +static int +qemuBuildRNGBackendArgs(virCommandPtr cmd, + virDomainRNGDefPtr dev, + virQEMUCapsPtr qemuCaps) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + int ret = -1; + + switch ((enum virDomainRNGBackend) dev->backend) { + case VIR_DOMAIN_RNG_BACKEND_RANDOM: + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_RNG_RANDOM)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("this qemu doesn't support the rng-random " + " backend")); + goto cleanup; + } + + virBufferAsprintf(&buf, "rng-random,id=%s", dev->info.alias); + if (dev->source.file) + virBufferAsprintf(&buf, ",filename=%s", dev->source.file); + + virCommandAddArg(cmd, "-object"); + virCommandAddArgBuffer(cmd, &buf); + break; + + case VIR_DOMAIN_RNG_BACKEND_EGD: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("egd RNG backend not yet implemented")); + goto cleanup; + break; + + case VIR_DOMAIN_RNG_BACKEND_LAST: + break; + } + + ret = 0; + +cleanup: + virBufferFreeAndReset(&buf); + return ret; +} + + +static int +qemuBuildRNGDeviceArgs(virCommandPtr cmd, + virDomainRNGDefPtr dev, + virQEMUCapsPtr qemuCaps) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + int ret = -1; + + if (dev->model != VIR_DOMAIN_RNG_MODEL_VIRTIO || + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_RNG)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("this qemu doesn't support RNG device type '%s'"), + virDomainRNGModelTypeToString(dev->model)); + goto cleanup; + } + + virBufferAsprintf(&buf, "virtio-rng-pci,rng=%s", dev->info.alias); + + if (qemuBuildDeviceAddressStr(&buf, &dev->info, qemuCaps) < 0) + goto cleanup; + + virCommandAddArg(cmd, "-device"); + virCommandAddArgBuffer(cmd, &buf); + + ret = 0; + +cleanup: + virBufferFreeAndReset(&buf); + return ret; +} + + static char *qemuBuildSmbiosBiosStr(virSysinfoDefPtr def) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -7006,6 +7094,16 @@ qemuBuildCommandLine(virConnectPtr conn, } } + if (def->rng) { + /* add the RNG source backend */ + if (qemuBuildRNGBackendArgs(cmd, def->rng, qemuCaps) < 0) + goto error; + + /* add the device */ + if (qemuBuildRNGDeviceArgs(cmd, def->rng, qemuCaps) < 0) + goto error; + } + if (snapshot) virCommandAddArgList(cmd, "-loadvm", snapshot->def->name, NULL); -- 1.8.1.1

On 02/21/2013 07:47 AM, Peter Krempa wrote:
This patch implements support for the virtio-rng-pci device and the rng-random backend in qemu.
Two capabilities bits are added to track support for those:
QEMU_CAPS_DEVICE_VIRTIO_RNG - for the device support and QEMU_CAPS_OBJECT_RNG_RANDOM - for the backend support.
Is there ever going to be a qemu with backend support but not the device? I guess the opposite is possible though - having the device, but being built with only one backend out of a set of backends available in qemu source code. I guess keeping this as two capability bits makes sense.
qemu is invoked with these additional parameters if the device is enabled:
-object rng-random,id=rng0,filename=/test/phile (to add the backend) -device virtio-rng-pci,rng=rng0,bus=pci.0,addr=0x4 (to add the device) ---
Notes: Version 2: - don't use different error messages for similar issues
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

This patch adds a new capability bit QEMU_CAPS_OBJECT_RNG_EGD and code to support the egd backend for the VirtIO RNG device. The device is added by 3 qemu command line options: -chardev socket,id=charrng0,host=1.2.3.4,port=1234 (communication backend) -object rng-egd,chardev=charrng0,id=rng0 (RNG protocol client) -device virtio-rng-pci,rng=rng0,bus=pci.0,addr=0x4 (the RNG device) --- Notes: Version 2: - ACKed, no change, doesn't make sense to push alone src/qemu/qemu_capabilities.c | 3 +++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 22 +++++++++++++++++++--- 3 files changed, 23 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index a3d9e06..02ce4bd 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -207,6 +207,8 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "add-fd", "virtio-rng", "rng-random", + + "rng-egd", /* 130 */ ); struct _virQEMUCaps { @@ -1332,6 +1334,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { { "usb-net", QEMU_CAPS_DEVICE_USB_NET}, { "virtio-rng-pci", QEMU_CAPS_DEVICE_VIRTIO_RNG }, { "rng-random", QEMU_CAPS_OBJECT_RNG_RANDOM }, + { "rng-egd", QEMU_CAPS_OBJECT_RNG_EGD }, }; diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index ee0d0ca..53dbcac 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -169,6 +169,7 @@ enum virQEMUCapsFlags { QEMU_CAPS_DEVICE_VIRTIO_RNG = 128, /* virtio-rng device */ QEMU_CAPS_OBJECT_RNG_RANDOM = 129, /* the rng-random backend for virtio rng */ + QEMU_CAPS_OBJECT_RNG_EGD = 130, /* EGD protocol daemon for rng */ QEMU_CAPS_LAST, /* this must always be the last item */ }; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 4a51413..1c9bfc9 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4184,6 +4184,7 @@ qemuBuildRNGBackendArgs(virCommandPtr cmd, virQEMUCapsPtr qemuCaps) { virBuffer buf = VIR_BUFFER_INITIALIZER; + char *backend = NULL; int ret = -1; switch ((enum virDomainRNGBackend) dev->backend) { @@ -4204,9 +4205,23 @@ qemuBuildRNGBackendArgs(virCommandPtr cmd, break; case VIR_DOMAIN_RNG_BACKEND_EGD: - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("egd RNG backend not yet implemented")); - goto cleanup; + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_RNG_EGD)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("this qemu doesn't support the rng-egd " + "backend")); + goto cleanup; + } + + if (!(backend = qemuBuildChrChardevStr(dev->source.chardev, + dev->info.alias, qemuCaps))) + goto cleanup; + + virCommandAddArgList(cmd, "-chardev", backend, NULL); + + virCommandAddArg(cmd, "-object"); + virCommandAddArgFormat(cmd, "rng-egd,chardev=char%s,id=%s", + dev->info.alias, dev->info.alias); break; case VIR_DOMAIN_RNG_BACKEND_LAST: @@ -4217,6 +4232,7 @@ qemuBuildRNGBackendArgs(virCommandPtr cmd, cleanup: virBufferFreeAndReset(&buf); + VIR_FREE(backend); return ret; } -- 1.8.1.1

On 02/21/2013 07:47 AM, Peter Krempa wrote:
This patch adds a new capability bit QEMU_CAPS_OBJECT_RNG_EGD and code to support the egd backend for the VirtIO RNG device.
The device is added by 3 qemu command line options: -chardev socket,id=charrng0,host=1.2.3.4,port=1234 (communication backend) -object rng-egd,chardev=charrng0,id=rng0 (RNG protocol client) -device virtio-rng-pci,rng=rng0,bus=pci.0,addr=0x4 (the RNG device) ---
Notes: Version 2: - ACKed, no change, doesn't make sense to push alone
Still good to go. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

Adds XML parsing and qemu commandline tests for the VirtIO RNG device support. --- Notes: Version 2: - ACKed .../qemuxml2argv-virtio-rng-egd.args | 1 + .../qemuxml2argv-virtio-rng-egd.xml | 26 ++++++++++++++++++++++ .../qemuxml2argv-virtio-rng-random.args | 1 + .../qemuxml2argv-virtio-rng-random.xml | 23 +++++++++++++++++++ tests/qemuxml2argvtest.c | 5 +++++ tests/qemuxml2xmltest.c | 3 +++ 6 files changed, 59 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-egd.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-egd.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-egd.args b/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-egd.args new file mode 100644 index 0000000..d40f0b5 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-egd.args @@ -0,0 +1 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefaults -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 -chardev socket,id=charrng0,host=1.2.3.4,port=1234 -object rng-egd,chardev=charrng0,id=rng0 -device virtio-rng-pci,rng=rng0,bus=pci.0,addr=0x4 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-egd.xml b/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-egd.xml new file mode 100644 index 0000000..4e52a32 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-egd.xml @@ -0,0 +1,26 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static' cpuset='1-4,8-20,525'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <controller type='usb' index='0'/> + <memballoon model='virtio'/> + <rng model='virtio'> + <backend model='egd' type='tcp'> + <source mode='connect' host='1.2.3.4' service='1234'/> + <protocol type='raw'/> + </backend> + </rng> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args b/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args new file mode 100644 index 0000000..4611ae5 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args @@ -0,0 +1 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefaults -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 -object rng-random,id=rng0,filename=/test/phile -device virtio-rng-pci,rng=rng0,bus=pci.0,addr=0x4 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml b/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml new file mode 100644 index 0000000..ab1f38c --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml @@ -0,0 +1,23 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static' cpuset='1-4,8-20,525'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <controller type='usb' index='0'/> + <memballoon model='virtio'/> + <rng model='virtio'> + <backend model='random'>/test/phile</backend> + </rng> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 4357068..b6b5489 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -882,6 +882,11 @@ mymain(void) QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_VIDEO_PRIMARY, QEMU_CAPS_DEVICE_QXL, QEMU_CAPS_DEVICE_QXL_VGA); + DO_TEST("virtio-rng-random", QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_VIRTIO_RNG, + QEMU_CAPS_OBJECT_RNG_RANDOM); + DO_TEST("virtio-rng-egd", QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_VIRTIO_RNG, + QEMU_CAPS_OBJECT_RNG_EGD); + virObjectUnref(driver.config); virObjectUnref(driver.caps); VIR_FREE(map); diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 3f36896..d64960f 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -242,6 +242,9 @@ mymain(void) DO_TEST("disk-scsi-disk-vpd"); + DO_TEST("virtio-rng-random"); + DO_TEST("virtio-rng-egd"); + /* These tests generate different XML */ DO_TEST_DIFFERENT("balloon-device-auto"); DO_TEST_DIFFERENT("channel-virtio-auto"); -- 1.8.1.1

On 02/21/2013 07:47 AM, Peter Krempa wrote:
Adds XML parsing and qemu commandline tests for the VirtIO RNG device support. ---
Notes: Version 2: - ACKed
.../qemuxml2argv-virtio-rng-egd.args | 1 + .../qemuxml2argv-virtio-rng-egd.xml | 26 ++++++++++++++++++++++ .../qemuxml2argv-virtio-rng-random.args | 1 + .../qemuxml2argv-virtio-rng-random.xml | 23 +++++++++++++++++++ tests/qemuxml2argvtest.c | 5 +++++ tests/qemuxml2xmltest.c | 3 +++ 6 files changed, 59 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-egd.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-egd.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml
Is it worth testing that a filename containing an XML-special character is properly escaped? Other than that, this one is still good to go.
+++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml @@ -0,0 +1,23 @@
+ <rng model='virtio'> + <backend model='random'>/test/phile</backend>
That is, should this use something like /test/<phile as the XML encoded file name? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 02/23/13 01:29, Eric Blake wrote:
On 02/21/2013 07:47 AM, Peter Krempa wrote:
Adds XML parsing and qemu commandline tests for the VirtIO RNG device support. ---
Notes: Version 2: - ACKed
.../qemuxml2argv-virtio-rng-egd.args | 1 + .../qemuxml2argv-virtio-rng-egd.xml | 26 ++++++++++++++++++++++ .../qemuxml2argv-virtio-rng-random.args | 1 + .../qemuxml2argv-virtio-rng-random.xml | 23 +++++++++++++++++++ tests/qemuxml2argvtest.c | 5 +++++ tests/qemuxml2xmltest.c | 3 +++ 6 files changed, 59 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-egd.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-egd.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml
Is it worth testing that a filename containing an XML-special character is properly escaped? Other than that, this one is still good to go.
+++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml @@ -0,0 +1,23 @@
+ <rng model='virtio'> + <backend model='random'>/test/phile</backend>
That is, should this use something like /test/<phile as the XML encoded file name?
Uh, I'm not following you on this one. You mean that if the user specifies some characters that are invalid from the perspective of XML as a source path? Anyways, I fixed the issues you pointed out in 1-6 and provided explanation for the other stuff. I'm pushing patches 1-7 (the test suite can be improved at any time) now and will follow up later with a improved version of 8 as well as with a patch that will allow multiple RNG devices. That should be better to review as slicing apart the existing patches. Peter

On 02/25/2013 03:45 AM, Peter Krempa wrote:
On 02/23/13 01:29, Eric Blake wrote:
On 02/21/2013 07:47 AM, Peter Krempa wrote:
Adds XML parsing and qemu commandline tests for the VirtIO RNG device support. ---
Is it worth testing that a filename containing an XML-special character is properly escaped? Other than that, this one is still good to go.
+++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml @@ -0,0 +1,23 @@
+ <rng model='virtio'> + <backend model='random'>/test/phile</backend>
That is, should this use something like /test/<phile as the XML encoded file name?
Uh, I'm not following you on this one. You mean that if the user specifies some characters that are invalid from the perspective of XML as a source path?
We should not get in the way of a user doing: ln -s /dev/urandom '/tmp/my<evil>random' then passing: <backend model='random'>/tmp/my<evil>random</backend> in their domain XML. By using virBufferEscape on the output side, we allow the user to use XML escapes on their input to specify any valid file name.
Anyways, I fixed the issues you pointed out in 1-6 and provided explanation for the other stuff. I'm pushing patches 1-7 (the test suite can be improved at any time) now and will follow up later with a improved version of 8 as well as with a patch that will allow multiple RNG devices. That should be better to review as slicing apart the existing patches.
Sure, doing things as followups is fine, since you already collected ACKs before my late review. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

Qemu's implementation of virtio RNG supports rate limiting of the entropy used. This patch exposes the option to tune this fucntionality. This patch is based on qemu commit 904d6f588063fb5ad2b61998acdf1e73fb4 The rate limiting is exported in the XML as: <devices> ... <rng model='virtio'> <rate period='1234'>4321</rate> <backend model='random'/> </rng> ... --- Notes: Version 2: - Qemu uses bytes/period, adapt the value according to that docs/formatdomain.html.in | 9 +++++++++ docs/schemas/domaincommon.rng | 18 +++++++++++++++++- src/conf/domain_conf.c | 17 +++++++++++++++++ src/conf/domain_conf.h | 2 ++ src/qemu/qemu_command.c | 9 +++++++++ .../qemuxml2argv-virtio-rng-random.args | 2 +- .../qemuxml2argv-virtio-rng-random.xml | 1 + 7 files changed, 56 insertions(+), 2 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index e8cd086..b264460 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4293,6 +4293,7 @@ qemu-kvm -net nic,model=? /dev/null ... <devices> <rng model='virtio'> + <rate period="2000">1234</rate> <backend model='random'>/dev/random</backend> <!-- OR --> <backend model='egd' type='udp'> @@ -4315,6 +4316,14 @@ qemu-kvm -net nic,model=? /dev/null <li>'virtio' — supported by qemu and virtio-rng kernel module</li> </ul> </dd> + <dt><code>rate</code></dt> + <dd> + <p> + The rate parameter allows to limit the rate that the entropy can be + read from the source. The value is in bits that the device is allowed + to read in the selected period. The default period is 1000ms or 1 second. + </p> + </dd> <dt><code>backend</code></dt> <dd> <p> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 8330a50..da53095 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3522,7 +3522,12 @@ <value>virtio</value> </choice> </attribute> - <ref name="rng-backend"/> + <interleave> + <ref name="rng-backend"/> + <optional> + <ref name="rng-rate"/> + </optional> + </interleave> </element> </define> @@ -3546,6 +3551,17 @@ </element> </define> + <define name="rng-rate"> + <element name="rate"> + <optional> + <attribute name="period"> + <ref name="positiveInteger"/> + </attribute> + </optional> + <ref name="positiveInteger"/> + </element> + </define> + <define name="usbmaster"> <element name="master"> <attribute name="startport"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b6de57c..ccf5200 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7501,6 +7501,17 @@ virDomainRNGDefParseXML(const xmlNodePtr node, ctxt->node = node; + if (virXPathUInt("string(./rate)", ctxt, &def->rate) < -1) { + virReportError(VIR_ERR_XML_ERROR, "%s", _("invalid RNG rate value")); + goto error; + } + + if (def->rate > 0 && + virXPathUInt("string(./rate/@period)", ctxt, &def->period) < -1) { + virReportError(VIR_ERR_XML_ERROR, "%s", _("invalid RNG period value")); + goto error; + } + if ((nbackends = virXPathNodeSet("./backend", ctxt, &backends)) < 0) goto error; @@ -13814,6 +13825,12 @@ virDomainRNGDefFormat(virBufferPtr buf, const char *backend = virDomainRNGBackendTypeToString(def->backend); virBufferAsprintf(buf, " <rng model='%s'>\n", model); + if (def->rate) { + virBufferAddLit(buf, " <rate"); + if (def->period) + virBufferAsprintf(buf, " period='%u'", def->period); + virBufferAsprintf(buf, ">%u</rate>\n", def->rate); + } virBufferAsprintf(buf, " <backend model='%s'", backend); switch ((enum virDomainRNGBackend) def->backend) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 0828954..4b62b93 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1736,6 +1736,8 @@ enum virDomainRNGBackend { struct _virDomainRNGDef { int model; int backend; + unsigned int rate; + unsigned int period; union { char *file; /* file name for 'random' source */ diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 1c9bfc9..6fcc093 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4255,6 +4255,15 @@ qemuBuildRNGDeviceArgs(virCommandPtr cmd, virBufferAsprintf(&buf, "virtio-rng-pci,rng=%s", dev->info.alias); + if (dev->rate > 0) { + /* qemu uses bytes */ + virBufferAsprintf(&buf, ",max-bytes=%u", dev->rate / 8); + if (dev->period) + virBufferAsprintf(&buf, ",period=%u", dev->period); + else + virBufferAddLit(&buf, ",period=1000"); + } + if (qemuBuildDeviceAddressStr(&buf, &dev->info, qemuCaps) < 0) goto cleanup; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args b/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args index 4611ae5..ced11db 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args @@ -1 +1 @@ -LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefaults -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 -object rng-random,id=rng0,filename=/test/phile -device virtio-rng-pci,rng=rng0,bus=pci.0,addr=0x4 +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefaults -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 -object rng-random,id=rng0,filename=/test/phile -device virtio-rng-pci,rng=rng0,max-bytes=100,period=1234,bus=pci.0,addr=0x4 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml b/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml index ab1f38c..26ddd38 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml @@ -17,6 +17,7 @@ <controller type='usb' index='0'/> <memballoon model='virtio'/> <rng model='virtio'> + <rate period='1234'>800</rate> <backend model='random'>/test/phile</backend> </rng> </devices> -- 1.8.1.1

On 02/21/2013 07:47 AM, Peter Krempa wrote:
Qemu's implementation of virtio RNG supports rate limiting of the entropy used. This patch exposes the option to tune this fucntionality.
s/fucntionality/functionality/
This patch is based on qemu commit 904d6f588063fb5ad2b61998acdf1e73fb4
The rate limiting is exported in the XML as: <devices> ... <rng model='virtio'> <rate period='1234'>4321</rate>
So period='nnn', if provided, is in milliseconds?
<backend model='random'/> </rng> ... ---
Notes: Version 2: - Qemu uses bytes/period, adapt the value according to that
@@ -4315,6 +4316,14 @@ qemu-kvm -net nic,model=? /dev/null <li>'virtio' — supported by qemu and virtio-rng kernel module</li> </ul> </dd> + <dt><code>rate</code></dt> + <dd> + <p> + The rate parameter allows to limit the rate that the entropy can be + read from the source. The value is in bits that the device is allowed + to read in the selected period. The default period is 1000ms or 1 second. + </p>
This documentation didn't quite make it clear that 'period' is an optional attribute in milliseconds. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 02/21/2013 09:47 AM, Peter Krempa wrote:
Version 2 fixes comments by John and several improvements I noticed. There's 1 new patch in this series. See patch notes for more info.
Peter Krempa (8): conf: Add fake switch statement to warn for new device types doc: schema: Add basic documentation for the virtual RNG device support conf: Add support for RNG device configuration in XML conf: Add RNG device ABI compatibility check qemu: Implement support for default 'random' backend for virtio-rng qemu: Implement support for EGD backend for virtio-rng tests: Add tests for virtio-rng device handling virtio-rng: Add rate limiting options for virtio-RNG
docs/formatdomain.html.in | 78 ++++++ docs/schemas/domaincommon.rng | 48 ++++ src/conf/domain_conf.c | 293 ++++++++++++++++++++- src/conf/domain_conf.h | 39 +++ src/libvirt_private.syms | 2 + src/qemu/qemu_capabilities.c | 6 + src/qemu/qemu_capabilities.h | 4 + src/qemu/qemu_command.c | 123 +++++++++ .../qemuxml2argv-virtio-rng-egd.args | 1 + .../qemuxml2argv-virtio-rng-egd.xml | 26 ++ .../qemuxml2argv-virtio-rng-random.args | 1 + .../qemuxml2argv-virtio-rng-random.xml | 24 ++ tests/qemuxml2argvtest.c | 5 + tests/qemuxml2xmltest.c | 3 + 14 files changed, 652 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-egd.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-egd.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml
I looked through the new set and all seems fine to me, although I'm not sure what to say about 1/8. John
participants (3)
-
Eric Blake
-
John Ferlan
-
Peter Krempa