[libvirt] [PATCH v1 2/6] conf: Parse and format shmem device XML

[Maxime Leroy]

On Tue, Aug 26, 2014 at 10:42 AM, Martin Kletzander <mkletzan at redhat.com> wrote:
> On Fri, Aug 22, 2014 at 12:47:01PM +0200, Maxime Leroy wrote:
>>
>> This patch adds configuration support for the shmem device
>> as described in the schema in the previous patch.
>>
[...]
>
> This parsing using loop over children does not readably (and in this
> case neither reliably) check whether required options are present.

For example, I could use: servernode = virXPathNode("./server", ctxt))
instead of having a loop ?

> Currently it parses <shmem name="asdf"/> as valid, but not specifying
> the size is probably not what you want on qemu command-line ... [1]

Size is an optional parameter of ivshmem in qemu. By default, the size is 4MB.
Thus <shmem name="asdf"/> is valid.

[...]
>> +                if (vectors &&
>> +                    virStrToLong_ui(vectors, NULL, 10, &def->msi.vectors)
>> < 0) {
>
>
> Use *_uip() if you don't want to accept negative values.
>

Ok. Thanks.

[...]
>> +static int virDomainIvshmemDefFormat(virBufferPtr buf,
>> +                                     virDomainIvshmemDefPtr def)
>> +{
>> +    if (def->server.enabled)
>> +        virBufferAsprintf(buf, "<server path='%s'/>\n",
>> +                          def->server.path);
>
>
> One general idea while looking through the code; could we make the
> path optional or even better, leave the "server" out somehow and
> generalize it even more.  The path could be for example
> "/var/run/libvirt/ivshmem-<name>-sock" since we are starting it
> anyway, and permissions on that would be easier to set then (whole
> path is prepared already).  The name would then be enough to get
> either the shmem name or the server socket path.

If libvirt starts the ivshmem server, then we can use a default path.

If libvirt did not start the server, then we should still let the path
as an optional field for cases where the user wants to start the
server himself with a custom path.

We can have an optional path field to handle both cases.

> Question is whether
> we can get the information whether server needs to be started from
> somewhere else.  E.g. does server make sense with no msi vectors and
> without ioeventfd?

The server handles eventfds distribution across ivshmem clients.

These eventfds are used to trigger interrupts (with or without msi) in
the "guests" clients.

So you can imagine starting a server without msi to only trigger
interrupts in the guests.

I would say that the server can be seen as a "support interrupt"
property of the shmem object but calling it server is more explicit.

>
>> +    if (def->size)
>> +        virBufferAsprintf(buf, "<size unit='M'>%llu</size>\n",
>> +                          def->size  / (1024 * 1024));
>> +
>
>
> If anyone specifies size < 1M, this won't be properly formatted
> (another idea for a test case).

Ivshmem in qemu only accepts size in MB or GB.

To prevent this error, we should not accept size under 1MB, in
virDomainIvshmemDefParseXML:

 if (virDomainParseScaledValue("./size[1]", ctxt,
 -                             &def->size, 1,
 +                             &def->size, 1024x1024,
                               ULLONG_MAX, true) < 0)

Maxime