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

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

On 08/13/2014 11:07 AM, Markus Armbruster wrote:
> Daniel H Barboza <danielhb@linux.vnet.ibm.com> writes:
>
>> 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?
> I can't see other block driver options with names of the form
> DRVNAME-OPTNAME, so keeping things standardized would rather call for
> just "timeout".
>
> Just "timeout" seems clear enough to me.  But if you can think of
> concrete contexts where it would be confusing, then I don't mind
> "curl-timeout".
>
I think you have a point. Since I can't think of any concrete context where
it would be confusing, I'll rename the option to "timeout" in v3.


Thanks!