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

[Martin Kletzander]

On Wed, Aug 27, 2014 at 10:52:02PM +0200, Maxime Leroy wrote:
>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 ?
>

Yes, sure.  I think that the parsing parts using the loops are
some very old code leftovers.  Maybe from some SAX parsing, but I
don't remember that, just guessing.

>> 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.
>

Oh, I didn't know that.  That is OK, but it should be mentioned in the
documentation, and tests for that should be included as well.

>[...]
>>> +                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.
>

I agree.
...
[coming back after few minutes]
...
Maybe you were right the first time.  Since we're not starting the
server, the path should be specified all the time.  After we add the
support for the server, we can relax the checks for the XML parsing.
That's always possible, but impossible the other way around (i.e. we
cannot make the checks more strict in the future).  So I'd say keep
the path mandatory and relax it in thelast patch that adds the support
for start='yes'.

One more suggestion that would help us get the patches earlier (before
the ivshmem-server is packaged in qemu) would be to check whether the
start='' attribute is in the XML and forbid such configuration.  And
also relax that check after the support for ivshmem-server is added.
Would you be OK with that?  What's you opinion?

>> 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.
>

That explicit naming is what I wanted to get rid of, originally (why I
asked the question above).  I'm still thinking about the
generalization of "shared memory" instead of exactly matching one
particular technology+hypervisor (qemu+ivshmem).

>>
>>> +    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.
>

Well, that should be somehow written in the documentation, then.  But
if I'm reading David's patch properly (not that I'm qemu-experienced
or anything), it might be smaller after his patches:

https://lists.gnu.org/archive/html/qemu-devel/2014-08/msg01243.html

We should somehow be able to parse any size in src/conf/domain_conf.c
(global parsing code), but PostParse() functions for QEMU
(src/qemu/qemu_domain.c) should allow only what's possible in QEMU.
Or even better, if we can introspect how qemu parses the size (whether
size=1 means 1 B or 1 MB; or whether it supports size=1K for example),
we can forbid invalid values in qemuBuildCommandLine() based on the
qemu that we'll be running.

Martin

>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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140828/0b338b04/attachment-0001.sig>