| Message ID | 27e0bf9dfb1370e8beb08c7b5ad6894540fff13a.1510093478.git.jcody@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/07/2017 04:27 PM, 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 | 37 ++++++++++++++++++++++++++----------- > 1 file changed, 26 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); Maybe s/with %d/with status %d/ > + return ret; This is returning a non-zero value, but not necessarily a negative errno... > @@ -821,8 +839,13 @@ static int ssh_create(const char *filename, QemuOpts *opts, Error **errp) > BDRVSSHState s; > ssize_t r2; > char c[1] = { '\0' }; > + 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; ...but this function wants to return a negative errno. I think you can rewrite this to: if (ssh_state_init(&s, errp)) { return -EIO; } and skip out on local_err.
On Tue, Nov 07, 2017 at 05:27:18PM -0500, 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 | 37 ++++++++++++++++++++++++++----------- > 1 file changed, 26 insertions(+), 11 deletions(-) > > diff --git a/block/ssh.c b/block/ssh.c > index b049a16..de81ec8 100644 > --- a/block/ssh.c > +++ b/block/ssh.c > @@ -83,12 +83,28 @@ typedef struct BDRVSSHState { > bool unsafe_flush_warning; > } BDRVSSHState; > > -static void ssh_state_init(BDRVSSHState *s) > +static bool ssh_libinit_called; > + > +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; > + } > + ssh_libinit_called = true; > + } > + > + > memset(s, 0, sizeof *s); > s->sock = -1; > s->offset = -1; > qemu_co_mutex_init(&s->lock); > + > + return 0; Is this thread safe? Is there a case where multiple ssh URL opens could race off against each other and we could end up calling libssh2_init in parallel? (According to the libssh2_init documentation, the function is not thread-safe so should not be called in parallel on multiple threads) Rich. > } > > static void ssh_state_free(BDRVSSHState *s) > @@ -773,7 +789,9 @@ static int ssh_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags, > int ret; > int ssh_flags; > > - ssh_state_init(s); > + if (ssh_state_init(s, errp)) { > + return -EIO; > + } > > ssh_flags = LIBSSH2_FXF_READ; > if (bdrv_flags & BDRV_O_RDWR) { > @@ -821,8 +839,13 @@ static int ssh_create(const char *filename, QemuOpts *opts, Error **errp) > BDRVSSHState s; > ssize_t r2; > char c[1] = { '\0' }; > + 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; > + } > > /* Get desired file size. */ > total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0), > @@ -1213,14 +1236,6 @@ static BlockDriver bdrv_ssh = { > > static void bdrv_ssh_init(void) > { > - int r; > - > - r = libssh2_init(0); > - if (r != 0) { > - fprintf(stderr, "libssh2 initialization failed, %d\n", r); > - exit(EXIT_FAILURE); > - } > - > bdrv_register(&bdrv_ssh); > } > > -- > 2.9.5
diff --git a/block/ssh.c b/block/ssh.c index b049a16..de81ec8 100644 --- a/block/ssh.c +++ b/block/ssh.c @@ -83,12 +83,28 @@ typedef struct BDRVSSHState { bool unsafe_flush_warning; } BDRVSSHState; -static void ssh_state_init(BDRVSSHState *s) +static bool ssh_libinit_called; + +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; + } + ssh_libinit_called = true; + } + + memset(s, 0, sizeof *s); s->sock = -1; s->offset = -1; qemu_co_mutex_init(&s->lock); + + return 0; } static void ssh_state_free(BDRVSSHState *s) @@ -773,7 +789,9 @@ static int ssh_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags, int ret; int ssh_flags; - ssh_state_init(s); + if (ssh_state_init(s, errp)) { + return -EIO; + } ssh_flags = LIBSSH2_FXF_READ; if (bdrv_flags & BDRV_O_RDWR) { @@ -821,8 +839,13 @@ static int ssh_create(const char *filename, QemuOpts *opts, Error **errp) BDRVSSHState s; ssize_t r2; char c[1] = { '\0' }; + 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; + } /* Get desired file size. */ total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0), @@ -1213,14 +1236,6 @@ static BlockDriver bdrv_ssh = { static void bdrv_ssh_init(void) { - int r; - - r = libssh2_init(0); - if (r != 0) { - fprintf(stderr, "libssh2 initialization failed, %d\n", r); - exit(EXIT_FAILURE); - } - bdrv_register(&bdrv_ssh); }
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 | 37 ++++++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 11 deletions(-)