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

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

On 08/13/2014 09:39 AM, Daniel H Barboza wrote:
>
> On 08/13/2014 06:15 AM, Markus Armbruster wrote:
>> Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> writes:
>>
>>> 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"
>> To what could this timeout apply other than Curl?
>>
>> If nothing, then just "timeout", please.
>>
>> Else, "curl-timeout".
>
> I will investigate a little to check if there are any option called 
> "timeout" used elsewhere. If not, I see no issues rename this option 
> to "timeout".
>
> Thanks!

I've found one option that contains the string "timeout":

vl.c

static QemuOptsList qemu_boot_opts = {
     .name = "boot-opts",
     .implied_opt_name = "order",
(...)
         }, {
             .name = "reboot-timeout",
             .type = QEMU_OPT_STRING,
         }, {
(...)

I think that renaming "curltimeout" to just "timeout" can be a little 
misleading depending on the context. However, I believe that to keep the 
name of the options standardized we can rename "curltimeout" to 
"curl-timeout" as you've suggested. Do you agree?


Thanks

>
>>>     struct BDRVCURLState;
>>>   @@ -109,6 +111,7 @@ typedef struct BDRVCURLState {
>>>       char *url;
>>>       size_t readahead_size;
>>>       bool sslverify;
>>> +    int curltimeout;
>>>       bool accept_range;
>>>       AioContext *aio_context;
>>>   } BDRVCURLState;
>> Likewise: either timeout, or curl_timeout.
>>
>> [...]
>>
>
>