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

[Markus Armbruster]

Daniel H Barboza <address@hidden> 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 <address@hidden> 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 <address@hidden>
>>>> ---
>>>>   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".