On 15.04.2015 16:38, Pavel Boldin wrote:
> Michal,
>
> On Wed, Apr 15, 2015 at 10:54 AM, Michal Privoznik <mprivozn@redhat.com>
> wrote:
>
>> On 26.03.2015 15:48, Pavel Boldin wrote:
>>> Dear Libvirt Developers,
>>>
>>>
>>> I'm working to implement feature request [1]. The feature request
>> proposes
>>> to enhance `libvirt' code so the API caller can specify which block
>> devices
>>> are to be migrated using e.g. parameters in the `virDomainMigrateToURI3'
>>> call.
>>
>> Correct. My idea is to add pairs of typed parameters to the
>> virDomainMigrate3() and virDomainMigrateToURI3() API. These are the only
>> migrate APIs that accept virTypedParameter. The rest of migrate APIs are
>> just too old and not extensible, so they must continue working as they
>> are now. However, those two APIs - they can be passed new pairs, e.g.
>> ("migrate_disk", "vda"). Now, mgmt applications will want to specify
>> more than one disk to migrate, obviously. We have two options:
>>
>> 1) make "migrate_disk" accept stringyfied array of disk targets, e.g.
>> "migrate_disk": "vda,vdb,sda". This is not so pleasant to work with from
>> client applications. Constructing the string would be awful.
>>
> My concern was that some drivers may allow commas inside the drive names.
> So this is obviously wrong thing to do then.

That's why I've chosen to work purely with disk target at NBD level. We
have strong rules what characters can occur there. Moreover, it's fairly
easy to derive qemu disk ID from the target. Oh, and we require targets
to be unique throughout the domain. So I think it's the best option for
the extension you're planning.

>
>
>>
>> 2) make virTypedParam* APIs accept more than one values to a key, e.g.
>> {("migrate_disk", "vda"), ("migrate_disk", "vdb"), ("migrate_disk",
>> "sda")}. Currently, this is supported on RPC, but some virTypedParam*
>> APIs are not really prepared for this. If I had to name some from the
>> top of my head: virTypedParamsGet().
>>
> As far as I see there is a line 420 in src/util/virtypedparam.c:
> /* The following APIs are public and their signature may never change. */

Of course. They would still return the first value found. But that's not
a problem. The paragraph is more like an intro than a suggestion to
change them.

>
> So, the functions need to be implemented anew.

Some of them. Correct.

>
>
>
>>
>> I view virTyped* as an associative array. So 2) is practically enabling
>> multi value associative array. So I guess the first step would be to
>> introduce new API (maybe set of APIs?) that are aware of this.
>>
> I guess the first function to implement would be virTypedParamsGetNth()
> that is able to retrieve nth parameter with that name.
> While this is easy to implement there are details remain:
>
>    1. Should we implement a new set of virTypedParamsGetNth<TYPE> for each
>    of the TYPE or should we start with just the 'String'?
>    2. Since virTypedParamsGet<TYPE> is just a virTypedParamsGetNth<TYPE>
>    with N=0 should we make old functions just call new ones with N=0?
>

Hm.. what about this, introduce just this new function:

virTypedParamsGetArrayForKey(.., const char *key, ...)

which will iterate through an array of typed params, producing a new
array which will, however, contain only the selected key. For instance,
if called over

{(key1, val1), (key1, val2), (key2, val3), (key4, val4)}

with key==key1 it would create array

{(key1, val1), (key1, val2)}

with key==key2 it would create array

{(key2, val3)}

And so on. For selecting Nth item in the array, we can just use basic C
array selector []. Oh, I don't like the function name much btw, but it
serves the example sake here.

The other option would be to have a different function:

char **
virTypedParamGetStringList( ..., const char *key)

which will produce a string list of values assigned with the @key. The
list will be NULL terminated of course. Again, the function name is
horrible, but it's just an example.

In fact, the latter approach would be much more easier to use, so I vote
for it.

Oh, and these functions I came up with - they don't even need to be
public! We can keep them private and live happily ever after. Well, not
quite - the virTypedParamsValidate() will require some changes too, but
whatever.

>
>
>>
>> Then, virDomainMigrate*3() can just use this in the following way:
>>
>> a) when no "migrate_disk" is specified, the current behaviour is preserved
>>
>> b) when there are some disk selected for storage migration, since at
>> this point we have virTyped* APIs to work with multivalue, we can get
>> the array of disk targets to migrate and instruct qemu to just migrate
>> them.
>>
>> Now, qemu has split storage and internal state migration into two
>> streams. The first one is called NBD and libvirt selectively choses
>> disks to migrate. For the other stream you don't have to care about.
>> Look at qemuMigrationDriveMirror() and you'll see how libvirt instructs
>> qemu to selectively migrate only some disks. The "migrate_disk" array
>> would need to be propagated down here then.
>>
> My concern is I would, most likely, have to backport these to our versions.

I'm not sure onto how old release, but in general, the smaller the patch
is, the easier to backport it is also ;-)

Michal