Re: [Qemu-devel] [PATCH v2] block.curl: adding 'curltimeout' option

[Daniel H Barboza <danielhb@linux.vnet.ibm.com>]

On 08/13/2014 11:38 AM, Benoît Canet wrote:
> The Tuesday 12 Aug 2014 à 16:12:57 (-0300), Daniel Henrique Barboza wrote :
>> The curl hardcoded timeout (5 seconds) sometimes is not long
>> enough depending on the remote server configuration and network
>> traffic. The user should be able to set how much long he is
>> willing to wait for the connection.
>>
>> Adding a new option to set this timeout gives the user this
>> flexibility. The previous default timeout of 5 seconds will be
>> used if this option is not present.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
>> ---
>>   block/curl.c    | 13 ++++++++++++-
>>   qemu-options.hx | 10 ++++++++--
>>   2 files changed, 20 insertions(+), 3 deletions(-)
>>
>> diff --git a/block/curl.c b/block/curl.c
>> index 79ff2f1..a9e43f1 100644
>> --- a/block/curl.c
>> +++ b/block/curl.c
>> @@ -63,6 +63,7 @@ static CURLMcode __curl_multi_socket_action(CURLM *multi_handle,
>>   #define CURL_NUM_ACB    8
>>   #define SECTOR_SIZE     512
>>   #define READ_AHEAD_DEFAULT (256 * 1024)
>> +#define CURL_TIMEOUT_DEFAULT 5
>>   
>>   #define FIND_RET_NONE   0
>>   #define FIND_RET_OK     1
>> @@ -71,6 +72,7 @@ static CURLMcode __curl_multi_socket_action(CURLM *multi_handle,
>>   #define CURL_BLOCK_OPT_URL       "url"
>>   #define CURL_BLOCK_OPT_READAHEAD "readahead"
>>   #define CURL_BLOCK_OPT_SSLVERIFY "sslverify"
>> +#define CURL_BLOCK_OPT_TIMEOUT "curltimeout"
>>   
>>   struct BDRVCURLState;
>>   
>> @@ -109,6 +111,7 @@ typedef struct BDRVCURLState {
>>       char *url;
>>       size_t readahead_size;
>>       bool sslverify;
>> +    int curltimeout;
> I know the sslverify does an exception to the coding style rule but why not
> name the field curl_timeout like it would be in every other part of qemu code ?
I'll change the name of the field to "timeout" in v3 as suggested by 
Markus Armbruster.
This fixes the coding style issue you've pointed out.

Thanks!

>
>>       bool accept_range;
>>       AioContext *aio_context;
>>   } BDRVCURLState;
>> @@ -382,7 +385,7 @@ static CURLState *curl_init_state(BDRVCURLState *s)
>>           curl_easy_setopt(state->curl, CURLOPT_URL, s->url);
>>           curl_easy_setopt(state->curl, CURLOPT_SSL_VERIFYPEER,
>>                            (long) s->sslverify);
>> -        curl_easy_setopt(state->curl, CURLOPT_TIMEOUT, 5);
>> +        curl_easy_setopt(state->curl, CURLOPT_TIMEOUT, s->curltimeout);
>>           curl_easy_setopt(state->curl, CURLOPT_WRITEFUNCTION,
>>                            (void *)curl_read_cb);
>>           curl_easy_setopt(state->curl, CURLOPT_WRITEDATA, (void *)state);
>> @@ -489,6 +492,11 @@ static QemuOptsList runtime_opts = {
>>               .type = QEMU_OPT_BOOL,
>>               .help = "Verify SSL certificate"
>>           },
>> +        {
>> +            .name = CURL_BLOCK_OPT_TIMEOUT,
>> +            .type = QEMU_OPT_NUMBER,
>> +            .help = "Curl timeout"
>> +        },
>>           { /* end of list */ }
>>       },
>>   };
>> @@ -525,6 +533,9 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
>>           goto out_noclean;
>>       }
>>   
>> +    s->curltimeout = qemu_opt_get_number(opts, CURL_BLOCK_OPT_TIMEOUT,
>> +                                         CURL_TIMEOUT_DEFAULT);
>> +
>>       s->sslverify = qemu_opt_get_bool(opts, CURL_BLOCK_OPT_SSLVERIFY, true);
>>   
>>       file = qemu_opt_get(opts, CURL_BLOCK_OPT_URL);
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index 96516c1..0358f38 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -2351,6 +2351,11 @@ multiple of 512 bytes. It defaults to 256k.
>>   @item sslverify
>>   Whether to verify the remote server's certificate when connecting over SSL. It
>>   can have the value 'on' or 'off'. It defaults to 'on'.
>> +
>> +@item curltimeout
>> +Set the timeout in seconds of the CURL connection. This timeout is the time
>> +that CURL waits for a response from the remote server to get the size of the
>> +image to be downloaded. If not set, the default timeout of 5 seconds is used.
>>   @end table
>>   
>>   Note that when passing options to qemu explicitly, @option{driver} is the value
>> @@ -2372,9 +2377,10 @@ qemu-system-x86_64 -drive file=/tmp/Fedora-x86_64-20-20131211.1-sda.qcow2,copy-o
>>   @end example
>>   
>>   Example: boot from an image stored on a VMware vSphere server with a self-signed
>> -certificate using a local overlay for writes and a readahead of 64k
>> +certificate using a local overlay for writes, a readahead of 64k and a
>> +curltimeout of 10 seconds.
>>   @example
>> -qemu-img create -f qcow2 -o backing_file='json:@{"file.driver":"https",, "file.url":"https://user:password@@vsphere.example.com/folder/test/test-flat.vmdk?dcPath=Datacenter&dsName=datastore1",, "file.sslverify":"off",, "file.readahead":"64k"@}' /tmp/test.qcow2
>> +qemu-img create -f qcow2 -o backing_file='json:@{"file.driver":"https",, "file.url":"https://user:password@@vsphere.example.com/folder/test/test-flat.vmdk?dcPath=Datacenter&dsName=datastore1",, "file.sslverify":"off",, "file.readahead":"64k",, "file.curltimeout":10@}' /tmp/test.qcow2
>>   
>>   qemu-system-x86_64 -drive file=/tmp/test.qcow2
>>   @end example
>> -- 
>> 1.8.3.1
>>
>>
> Apart from the coding style issue.
>
> Reviewed-by: Benoit Canet <benoit.canet@nodalink.com>
>