[libvirt] [PATCH 3/4] conf: Add basic support for RNG configuration

[Peter Krempa]

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