[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v2 1/7] block/ssh: don't call libss
From: |
Jeff Cody |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v2 1/7] block/ssh: don't call libssh2_init() in block_init() |
Date: |
Wed, 30 Aug 2017 16:11:07 -0400 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
On Wed, Aug 30, 2017 at 02:40:16PM -0500, Eric Blake wrote:
> 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 <address@hidden>
> > ---
> > 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?
>
>From the documentation [1], it returns 0 on success, or a negative value
for error. (I guess presumably that means a positive value is by definition
an error, as well).
[1] https://www.libssh2.org/libssh2_init.html
> > @@ -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;
> }
Good point, not sure if a non-zero ret from libssh2_init() provides meaning
beyond 'error' for us. Maybe return -EIO instead of -1, though?
- [Qemu-block] [PATCH v2 0/7] Code cleanup and minor fixes, Jeff Cody, 2017/08/30
- [Qemu-block] [PATCH v2 3/7] block/sheepdog: remove spurious NULL check, Jeff Cody, 2017/08/30
- [Qemu-block] [PATCH v2 2/7] block/ssh: make compliant with coding guidelines, Jeff Cody, 2017/08/30
- [Qemu-block] [PATCH v2 5/7] block/curl: check error return of curl_global_init(), Jeff Cody, 2017/08/30
- [Qemu-block] [PATCH v2 6/7] block/curl: fix minor memory leaks, Jeff Cody, 2017/08/30