On 01/28/13 23:27, Eric Blake wrote:

On 01/11/2013 10:00 AM, Peter Krempa wrote:

...

This patch documments XML elements used for (basic) support of virtual

s/documments/documents/

...

RNG devices.

In the devices section in the domain XML users may specify:

<devices> <rng model='none'/>

Do we need model='none'? Historically, we have added it for devices where the device used to be always-on by default, but we didn't have XML for the device, then later on the device became optional so we had to have a way to express the absence of the device without breaking back-compat of old XML that gets the default for free. But in the case of rng, I don't think we currently have any device for free, so model='none' doesn't seem to add anything.

Hm, I was using the none model for testing purposes and decided to leave it in. I found it useful to be able to disable the device without having to delete the complete definition. I agree that the none model is not strictly needed to be formatted in the XML but I would still leave it as a option to be parsed.

...

</devices>

and the more useful variant:

<devices> <rng model='virtio'> <source type='random'>/dev/urandom</source> </rng> </devices>

This part, however, seems reasonable.

...

--- docs/formatdomain.html.in | 54 +++++++++++++++++++++++++++++++++++++++++++ docs/schemas/domaincommon.rng | 31 +++++++++++++++++++++++++ 2 files changed, 85 insertions(+)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index bb0b199..7a5f267 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4260,6 +4260,60 @@ 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.2</span>

Yikes - our delay in reviewing this means that it might not make 1.0.2 after all. It's probably a good sign that the list is now getting so many patches, but at the same time it points out that everyone needs to help more with reviews.

It's not that important for 1.0.2 anyways. I wanted to start a round of reviews as I'm planing on adding other backends and stuff but I wanted to do the initial design right.

...

+ <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>'none' — disable the rng device</li>

Again, isn't omitting the <rng> element sufficient for this task?

It is.

...

+ <li>'virtio' — supported by qemu and virtio-rng kernel module</li> + </ul> + </dd> + <dt><code>source</code></dt> + <dd> + <p> + The <code>source</code> element specifies the source of entropy + to be used for the doimain. The source type is configured using the

s/doimain/domain/

...

+ <code>type</code> attribute. Supported source types are: + </p> + <ul> + <li>'none' — no source was configured</li> + <li>'random' — /dev/random or similar device as source</li>

Here, it _does_ make sense to expose both a type='none' and type='random'; assuming that qemu has a default source of entropy that it uses when libvirt doesn't specify one.

...

+ </ul> + </dd> + <dt><code>source type='random'</code></dt> + <dd> + <p> + This source 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>source</code> element.

Side question - is it also possible to pass a (large) regular file, so that you can experiment with reproducible random replays? At least with coreutil's 'shuf --random-source', it is indeed possible to replay a particular byte stream (obviously, on reply, the element of true randomness is gone, but for analysis purposes, it can still be a useful thing to do).

I haven't tried that configuration yet. It might be possible with the current state of qemu and it will be possible when I implement the entropy distribution daemon. There you will be able to configure a source either from the kernel's interfaces, hardware generators or a file for testing purposes.

I guess what this means for libvirt is that we should allow whatever file name the user passes, rather than actually stat()ing the file and enforcing that it be a char device.

I will try that and tweak this appropriately.

...

+ <define name="rng"> + <element name="rng"> + <attribute name="model"> + <choice> + <value>none</value> + <value>virtio</value> + </choice>

Again, not sure we need this level of <choice>.

virtio model will be sufficient for most use cases.

...

+ </attribute> + </element> + </define> + + <define name="rng-source"> + <element name="source"> + <choice> + <group> + <attribute name="type"> + <value>none</value> + </attribute> + <empty/> + </group> + <group> + <attribute name="type"> + <value>random</value> + </attribute> + <ref name="filePath"/> + </group> + </choice> + </element> + </define>

This part looks fine.

Peter

On 01/11/2013 10:00 AM, Peter Krempa wrote:

With this change it's easy to spot a forgotten free if a new device class is added. --- src/conf/domain_conf.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)

ACK; and this can go in pre-release.

- switch (def->type) { + switch ((virDomainDeviceType) def->type) { case VIR_DOMAIN_DEVICE_DISK: virDomainDiskDefFree(def->data.disk); break; @@ -1562,6 +1562,13 @@ void virDomainDeviceDefFree(virDomainDeviceDefPtr def) case VIR_DOMAIN_DEVICE_REDIRDEV: virDomainRedirdevDefFree(def->data.redirdev); break; + case VIR_DOMAIN_DEVICE_NONE: + case VIR_DOMAIN_DEVICE_FS: + case VIR_DOMAIN_DEVICE_SMARTCARD: + case VIR_DOMAIN_DEVICE_CHR: + case VIR_DOMAIN_DEVICE_MEMBALLOON: + case VIR_DOMAIN_DEVICE_LAST: + break; }

In general, this style is useful on ANY switch statement over a set of enums, for letting the compiler help us know when the enumeration grew. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 01/28/13 23:42, Eric Blake wrote:

On 01/11/2013 10:00 AM, Peter Krempa wrote:

...

This patch adds basic configuration support for the RNG device suporting the virtio model with the "random" backend type. --- src/conf/domain_conf.c | 148 ++++++++++++++++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 36 ++++++++++++ src/libvirt_private.syms | 2 + 3 files changed, 185 insertions(+), 1 deletion(-)

We're post-freeze before I reviewed this; on the other hand, I don't think that adding it now could cause too many issues. So unless anyone else speaks up in the next 24 hours, I think I could live with this making it into 1.0.2. Then again, I have enough comments on the series that it will be worth a v2.

I will not force this in 1.0.2.

...

+VIR_ENUM_IMPL(virDomainRNGModel, + VIR_DOMAIN_RNG_MODEL_LAST, + "none", + "virtio");

As in 1/4, I don't think we need this one, or at least, we don't need the 'none' value. As long as 'virtio' is the only model we support, we may not even need the model='xxx' attribute. It boils down to a question of future growth - will it be easier to have model='virtio' now and add a new model='foo' when (if?) we finally introduce a counterpart model, or would it be easier to not have model= at all for now, and add 'model=virtio|foo' later on where a missing model defaults to virtio at that time.

I don't particularly like default models. We might document that the default model is virtio but we certainly shouldn't rely on hypervisor default here. It proved to be a bad decision. And even when there's a single model we should always format it in the XML so that later we can change the default if it proves to be problematic and ensure that older XML's are compatible.

...

+ +VIR_ENUM_IMPL(virDomainRNGSource, + VIR_DOMAIN_RNG_SOURCE_LAST, + "none", + "random");

This one is fine.

...

+static virDomainRNGDefPtr +virDomainRNGDefParseXML(const xmlNodePtr node, + xmlXPathContextPtr ctxt, + unsigned int flags) +{ + const char *model; + const char *source; + virDomainRNGDefPtr def; + xmlNodePtr save = ctxt->node; + xmlNodePtr *sources = NULL; + int nsources; + + if (VIR_ALLOC(def) < 0) { + virReportOOMError(); + return NULL; + } + + if (!(model = virXMLPropString(node, "model"))) + model = "none"; + + if ((def->model = virDomainRNGModelTypeFromString(model)) < 0) { + virReportError(VIR_ERR_XML_ERROR, _("unknown RNG model '%s'"), model); + goto error; + } + + ctxt->node = node; + + if ((nsources = virXPathNodeSet("./source", ctxt, &sources)) < 0) + goto error; + + if (nsources > 1) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("only one RNG source is supported")); + goto error; + } + + if (nsources == 1 && + (source = virXMLPropString(sources[0], "type"))) { + if ((def->source = virDomainRNGSourceTypeFromString(source)) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown RNG source type '%s'"), source); + goto error; + } + + switch ((enum virDomainRNGSource) def->source) { + case VIR_DOMAIN_RNG_SOURCE_NONE:

If the user specified <rng><source type='none'>/path/to/file</source></rng>, should we error out that a file name is incompatible with type='none'?

Yeah. For normal usage this is not necessary. I used it mainly for easier testing.

...

@@ -10309,6 +10397,22 @@ static virDomainDefPtr 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 memory balloon device is supported"));

Too much copy and paste in this error message.

Ew :)

...

+static int +virDomainRNGDefFormat(virBufferPtr buf, + virDomainRNGDefPtr def) +{ + const char *model = virDomainRNGModelTypeToString(def->model); + const char *source = virDomainRNGSourceTypeToString(def->source); + + virBufferAsprintf(buf, " <rng model='%s'>\n", model); + virBufferAsprintf(buf, " <source type='%s'", source); + + switch ((enum virDomainRNGSource) def->source) { + case VIR_DOMAIN_RNG_SOURCE_LAST: + case VIR_DOMAIN_RNG_SOURCE_NONE: + virBufferAddLit(buf, "/>\n"); + break; + + case VIR_DOMAIN_RNG_SOURCE_RANDOM: + if (def->address) + virBufferAsprintf(buf, ">%s</source>\n", def->address);

You must use virBufferEscape(), as this is a user-supplied file name which might contain characters that would mess up valid XML if not escaped properly.

...

+void +virDomainRNGDefFree(virDomainRNGDefPtr def) +{ + if (!def) + return; + + VIR_FREE(def->address); + VIR_FREE(def->service);

We don't set def->service; why are we freeing it? Save that field for a patch that actually extends the XML to use it.

...

+++ b/src/conf/domain_conf.h @@ -115,6 +115,9 @@ typedef virDomainSnapshotObj *virDomainSnapshotObjPtr; typedef struct _virDomainSnapshotObjList virDomainSnapshotObjList; typedef virDomainSnapshotObjList *virDomainSnapshotObjListPtr;

...

+enum virDomainRNGModel { + VIR_DOMAIN_RNG_MODEL_NONE, + VIR_DOMAIN_RNG_MODEL_VIRTIO, + + VIR_DOMAIN_RNG_MODEL_LAST +}; + +enum virDomainRNGSource { + VIR_DOMAIN_RNG_SOURCE_NONE, + VIR_DOMAIN_RNG_SOURCE_RANDOM, + /* VIR_DOMAIN_RNG_SOURCE_EGD, */ + /* VIR_DOMAIN_RNG_SOURCE_POOL, */

Ah, I see where you plan to add future source types. Comments here are okay (as a hint about future development)...

...

+ + VIR_DOMAIN_RNG_SOURCE_LAST +}; + +struct _virDomainRNGDef { + int model; + int source;

Add /* virDomainRNGSource */, to make it obvious what this int will hold.

...

+ + char *address; /* file name/socket name/pool name/address */ + char *service; /* port, class name */

...but service seems like an unused field, and probably doesn't make sense to add until you actually implement those future source types.

Okay, I will add that later. Peter

On 01/11/2013 10:00 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 any version of qemu where one but not the other feature is available? Or are we always guaranteed that if one is present, the other is as well, in which case we really only need one bit, QEMU_CAPS_VIRTIO_RNG?

--- src/qemu/qemu_capabilities.c | 5 +- src/qemu/qemu_capabilities.h | 3 ++ src/qemu/qemu_command.c | 108 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 115 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index b166dd6..4947a3a 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -203,7 +203,8 @@ VIR_ENUM_IMPL(qemuCaps, QEMU_CAPS_LAST,

"usb-serial", /* 125 */ "usb-net", - + "virtio-rng", + "rng-random",

There may be trivial merge conflicts to resolve, but not a big deal.

+ case VIR_DOMAIN_RNG_SOURCE_RANDOM: + if (qemuCapsGet(caps, QEMU_CAPS_OBJECT_RNG_RANDOM)) { + virBufferAsprintf(&buf, "rng-random,id=%s", dev->info.alias); + if (dev->address) + virBufferAsprintf(&buf, ",filename=%s", dev->address); + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("this qemu doesn't support the rng-random " + " backend"));

Two spaces in the error message line wrap; should only be one.

+char * +qemuBuildRNGDevStr(virDomainRNGDefPtr dev, + qemuCapsPtr caps) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (dev->model != VIR_DOMAIN_RNG_MODEL_VIRTIO || + !qemuCapsGet(caps, QEMU_CAPS_DEVICE_VIRTIO_RNG)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("RNG device type '%s' is not supported " + "by this of qemu"),

Missing the word 'version'.

@@ -6978,6 +7064,28 @@ qemuBuildCommandLine(virConnectPtr conn, } }

+ if (def->rng && + def->rng->model != VIR_DOMAIN_RNG_MODEL_NONE) { + char *optstr;

I'm still not convinced we need def->rng->model of NONE; if def->rng exists, it should point to a valid model. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org