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

[Daniel H Barboza]

On 08/13/2014 11:07 AM, Markus Armbruster wrote:
> 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".
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!