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

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@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@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(a)redhat.com    +1-919-301-3266 Libvirt virtualization library http://libvirt.org