4:49 a.m.
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