[libvirt] [PATCH] Increased upper limit on lists of pool names

[Jesse Cook]

On Thu, Mar 15, 2012 at 11:47 AM, Eric Blake <eblake at redhat.com> wrote:
> On 03/15/2012 08:31 AM, Eric Blake wrote:
>> It may also be worth considering the addition of _just_ a new ranged RPC
>> call, but no new public API, by making the public API smart enough to
>> automatically call the ranged RPC multiple times to build up a single
>> return to the user.  After all, if we exposed the ranged call to the
>> user, then each client will have to add the logic to make the multiple
>> calls; whereas our current limitation is _only_ in the RPC aspect (once
>> the data is across the RPC and into the target process' memory space,
>> there's no longer a 64k cap to worry about).
>
> Thinking more about this idea:
>
> Suppose we have the following in remote_protocol.x:
>
> Existing:
>
> struct remote_storage_pool_list_volumes_args {
>    remote_nonnull_storage_pool pool;
>    int maxnames;
> };
> struct remote_storage_pool_list_volumes_ret {
>    remote_nonnull_string names<REMOTE_STORAGE_VOL_NAME_LIST_MAX>; /*
> insert at 1 */
> };
> ...
>    REMOTE_PROC_STORAGE_POOL_LIST_VOLUMES = 92, /* autogen skipgen
> priority:high */
>
> (note the change from autogen to skipgen for src/remote)
>
> New:
>
> struct remote_storage_pool_list_volumes_get_transaction_args {
>    remote_nonnull_storage_pool pool;
>    int maxnames;
> };
> struct remote_storage_pool_list_volumes_get_transaction_ret {
>    int transaction;
> };
> struct remote_storage_pool_list_volumes_transaction_args {
>    remote_nonnull_storage_pool pool;
>    int transaction;
>    int start;
> };
> struct remote_storage_pool_list_volumes_ret {
>    remote_nonnull_string names<REMOTE_STORAGE_VOL_NAME_LIST_MAX>; /*
> insert at 1 */
> };
> ...
>    REMOTE_PROC_STORAGE_POOL_LIST_VOLUMES_GET_TRANSACTION = ... /*
> skipgen skipgen priority:high */
>    REMOTE_PROC_STORAGE_POOL_LIST_VOLUMES_TRANSACTION = ... /* skipgen
> skipgen priority:high */
>
>
> Then, in pseudocode, we change the currently-generated src/remote
> handler for storagePoolListVolumes to do:
>
> if maxnames <= REMOTE_STORAGE_VOL_NAME_LIST_MAX
>    use REMOTE_PROC_STORAGE_POOL_LIST_VOLUMES /* existing code */
> else {
>    int transaction, i = 0;
>    pass pool and maxnames through
> REMOTE_PROC_STORAGE_POOL_LIST_VOLUMES_GET_TRANSACTION to set transaction
>    while (i < maxnames) {
>        pass pool and transaction through
> REMOTE_PROC_STORAGE_POOL_LIST_VOLUMES_TRANSACTION to get 128 more names
>        i += REMOTE_STORAGE_VOL_NAME_LIST_MAX;
>    }
> }
>
> Then, in daemon/remote.c, we leave the existing handler (which calls
> back into virStoragePoolListVolumes with 256 or less entries) untouched,
> and add two new hand-written RPC handlers:
>
> the handler for REMOTE_PROC_STORAGE_POOL_LIST_VOLUMES_GET_TRANSACTION:
>    register that a sequenced transaction has been started
>    allocate an array of maxnames and position and call
> virStoragePoolListVolumes, and tie that result to the sequence
>    return the sequence transaction number
>
> the handler for REMOTE_PROC_STORAGE_POOL_LIST_VOLUMES_TRANSACTION:
>    validates that the sequence number is valid, and that the start
> value matches the recorded position
>    prepares the return RPC with the next 128 names from the array tied
> to the sequence, and updates the position
>    if the position reached the end, free the array and end the
> sequenced transaction
>
> we also update virnetserver.c to support sequenced commands - the idea
> here is that any time a transaction will need more than one RPC message
> between client and server, then a transaction number can be generated
> which reserves memory tied to the transaction; as long as that
> transaction number exists, then further commands sent with reference to
> that transaction can reuse that memory; the transaction is then closed
> if the server and client complete the transaction, but also closed (and
> frees the reserved memory) if the connection disappears.  To avoid a
> misbehaving client from DoS'ing a server, we could also require that
> while a transaction is open, the next RPC call from the client must
> reference that transaction, and must come within a reasonable time
> limit.  To some extent, we already have a 'continue' status for a
> message; right now, a 'call' can only be replied by 'ok' or 'error', but
> this would be a case where 'call' is now replied by 'continue' to state
> that a transaction is in effect, and where we could reuse the 'serial'
> for as long as the transaction is in effect.
>
> At any rate, I don't have code to back this up, but it's certainly food
> for thought on whether it makes sense to try and enhance our use of RPC
> to support transactions without also having to add new public API.
>
> --
> Eric Blake   eblake at redhat.com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>

Your solution seems reasonable, though I'd have to really think about
it to see if there are any major holes in it.  Could you leverage
compression to eliminate the need for a more sophisticated solution?