Re: [Qemu-devel] [PATCH v2 1/7] block/ssh: don't call libssh2_init() in block_init()

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1535 bytes --]

On 08/30/2017 11:56 AM, Jeff Cody wrote:
> We don't need libssh2 failure to be fatal (we could just opt to not
> register the driver on failure). But, it is probably a good idea to
> avoid external library calls during the block_init(), and call the
> libssh2 global init function on the first usage, returning any errors.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/ssh.c | 40 +++++++++++++++++++++++++++++-----------
>  1 file changed, 29 insertions(+), 11 deletions(-)
> 

> +static int ssh_state_init(BDRVSSHState *s, Error **errp)
>  {
> +    int ret;
> +
> +    if (!ssh_libinit_called) {
> +        ret = libssh2_init(0);
> +        if (ret) {
> +            error_setg(errp, "libssh2 initialization failed with %d", ret);
> +            return ret;

Do we know if this number is always positive or negative?

> @@ -772,8 +788,13 @@ static int ssh_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags,
>      BDRVSSHState *s = bs->opaque;
>      int ret;
>      int ssh_flags;
> +    Error *local_err = NULL;
>  
> -    ssh_state_init(s);
> +    ret = ssh_state_init(s, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return ret;

Is returning 'ret' from libssh2_init() wise?

If 'ret' is not important, you could simplify this:

if (ssh_state_init(s, errp)) {
    return -1;
}

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]