Code cleanup and minor fixes
v2 cover Code cleanup and minor fixes
Minor bug fixes and code cleanup.
Changes from v1 -> v2:
Rebased to v2.10.0
Jeff Cody (7):
block/ssh: don't call libssh2_init() in block_init() block/ssh: make compliant with coding guidelines block/sheepdog: remove spurious NULL check block/sheepdog: code beautification block/curl: check error return of curl_global_init() block/curl: fix minor memory leaks block/curl: code cleanup to comply with coding style
block/curl.c | 124 +++++++++++++++++++++++------------------ block/sheepdog.c | 164 +++++++++++++++++++++++++++---------------------------- block/ssh.c | 72 +++++++++++++++--------- 3 files changed, 199 insertions(+), 161 deletions(-)
Signature · 1 line
2.9.5
PATCH v2 1/7 block/ssh: don't call libssh2_init() in block_init()
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.
--- block/ssh.c | 40 +++++++++++++++++++++++++++++----------- 1 file changed, 29 insertions(+), 11 deletions(-)
block/ssh.c +29 -11
@@ -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)
@@ -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; + } ssh_flags =LIBSSH2_FXF_READ ;if (bdrv_flags &BDRV_O_RDWR ) {
@@ -821,8 +842,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 +1239,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); }
Signature · 1 line
2.9.5
Re: [Qemu-devel] [PATCH v2 1/7] block/ssh: don't call libssh2_init() in block_init()
Quoted 11 lines · Jeff Cody
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 <[email protected]>--- block/ssh.c | 40 +++++++++++++++++++++++++++++----------- 1 file changed, 29 insertions(+), 11 deletions(-)
block/ssh.c typedef struct BDRVSSHState { · line 96
Quoted 9 lines
block/ssh.c +8
-0+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?
block/ssh.c static int ssh_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags, · line 796
Quoted 11 lines
block/ssh.c +5
-1@@ -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;
}Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH v2 1/7] block/ssh: don't call libssh2_init() in block_init()
Quoted 25 lines · Eric Blake
Quoted 11 lines · Jeff Cody
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 <[email protected]>--- block/ssh.c | 40 +++++++++++++++++++++++++++++----------- 1 file changed, 29 insertions(+), 11 deletions(-)block/ssh.c typedef struct BDRVSSHState { · line 96
Quoted 9 lines
block/ssh.c +8
-0+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?
Quoted 1 line
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
Quoted 14 lines
block/ssh.c static int ssh_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags, · line 796
Quoted 11 lines
block/ssh.c +5
-1@@ -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?
Quoted 5 lines
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?
Re: [Qemu-devel] [PATCH v2 1/7] block/ssh: don't call libssh2_init() in block_init()
Quoted 11 lines · Jeff Cody
Quoted 10 lines · Eric Blake
Quoted 5 lines · Jeff Cody
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.Is returning 'ret' from
libssh2_init()wise?
Quoted 8 lines
Quoted 5 lines
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-EIOinstead of -1, though?
You used -EIO for 5/7, so that does sound a bit better.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH v2 1/7] block/ssh: don't call libssh2_init() in block_init()
In reply to Eric BlakeQuoted 12 lines · Eric Blake
Quoted 10 lines · Jeff Cody
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 <[email protected]>--- block/ssh.c | 40 +++++++++++++++++++++++++++++----------- 1 file changed, 29 insertions(+), 11 deletions(-)
Quoted 11 lines
block/ssh.c typedef struct BDRVSSHState { · line 96
Quoted 9 lines
block/ssh.c +8
-0+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?
FWIW documentation says:
"Returns 0 if succeeded, or a negative value for error."
I was holding off on reviewing this patch in general since there's a libssh-based alternative driver under development, which should be better.
Rich.
Signature · 5 lines
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
PATCH v2 2/7 block/ssh: make compliant with coding guidelines
In reply to Jeff Cody--- block/ssh.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-)
block/ssh.c +18 -14
@@ -241,7 +241,7 @@ static int parse_uri(const char *filename, QDict *options, Error **errp) goto err; } -if (uri->user &&strcmp (uri->user,"" ) !=0 ) { +if (uri->user &&strcmp (uri->user,"" ) !=0 ) {qdict_put_str (options,"user" , uri->user); }
@@ -268,7 +268,7 @@ static int parse_uri(const char *filename, QDict *options, Error **errp) err:if (uri) { -uri_free (uri); +uri_free (uri); }return -EINVAL ; }
@@ -342,7 +342,7 @@ static int check_host_key_knownhosts(BDRVSSHState *s,libssh2_knownhost_readfile (knh, knh_file,LIBSSH2_KNOWNHOST_FILE_OPENSSH ); r =libssh2_knownhost_checkp (knh, host, port, hostkey, len, -LIBSSH2_KNOWNHOST_TYPE_PLAIN | +LIBSSH2_KNOWNHOST_TYPE_PLAIN |LIBSSH2_KNOWNHOST_KEYENC_RAW , &found);switch (r) {
@@ -405,15 +405,18 @@ static int compare_fingerprint(const unsigned char *fingerprint, size_t len,unsigned c;while (len >0 ) { -while (*host_key_check ==':' ) +while (*host_key_check ==':' ) { host_key_check++; + }if (!qemu_isxdigit (host_key_check[0 ]) || - !qemu_isxdigit (host_key_check[1 ])) + !qemu_isxdigit (host_key_check[1 ])) {return 1 ; + } c =hex2decimal (host_key_check[0 ]) *16 +hex2decimal (host_key_check[1 ]); -if (c - *fingerprint !=0 ) +if (c - *fingerprint !=0 ) {return c - *fingerprint; + } fingerprint++; len--; host_key_check +=2 ;
@@ -433,8 +436,8 @@ check_host_key_hash(BDRVSSHState *s, const char *hash,return -EINVAL ; } -if (compare_fingerprint ((unsigned char *) fingerprint, fingerprint_len, - hash) !=0 ) { +if (compare_fingerprint ((unsigned char *) fingerprint, fingerprint_len, + hash) !=0 ) {error_setg (errp,"remote host key does not match host_key_check '%s'" , hash);return -EPERM ;
@@ -507,7 +510,7 @@ static int authenticate(BDRVSSHState *s, const char *user, Error **errp) goto out; } -for (;;) { +for (;;) { r =libssh2_agent_get_identity (agent, &identity, prev_identity);if (r ==1 ) {/* end of list */ break ;
@@ -863,8 +866,8 @@ static int ssh_create(const char *filename, QemuOpts *opts, Error **errp) } r =connect_to_ssh (&s, uri_options, -LIBSSH2_FXF_READ |LIBSSH2_FXF_WRITE | -LIBSSH2_FXF_CREAT |LIBSSH2_FXF_TRUNC , +LIBSSH2_FXF_READ |LIBSSH2_FXF_WRITE | +LIBSSH2_FXF_CREAT |LIBSSH2_FXF_TRUNC ,0644 , errp);if (r <0 ) { ret = r;
@@ -872,7 +875,7 @@ static int ssh_create(const char *filename, QemuOpts *opts, Error **errp) }if (total_size >0 ) { -libssh2_sftp_seek64 (s.sftp_handle, total_size-1 ); +libssh2_sftp_seek64 (s.sftp_handle, total_size -1 ); r2 =libssh2_sftp_write (s.sftp_handle, c,1 );if (r2 <0 ) {sftp_error_setg (errp, &s,"truncate failed" );
@@ -1111,7 +1114,7 @@ static int ssh_write(BDRVSSHState *s, BlockDriverState *bs,* works for me. */ if (r ==0 ) { -ssh_seek (s, offset + written,SSH_SEEK_WRITE |SSH_SEEK_FORCE ); +ssh_seek (s, offset + written,SSH_SEEK_WRITE |SSH_SEEK_FORCE );co_yield (s, bs); goto again; }
@@ -1125,8 +1128,9 @@ static int ssh_write(BDRVSSHState *s, BlockDriverState *bs, end_of_vec = i->iov_base + i->iov_len; } -if (offset + written > s->attrs.filesize) +if (offset + written > s->attrs.filesize) { s->attrs.filesize = offset + written; + } }return 0 ;
Signature · 1 line
2.9.5
Re: [Qemu-devel] [PATCH v2 2/7] block/ssh: make compliant with coding guidelines
Quoted 4 lines · Jeff Cody
Signed-off-by Jeff Cody <[email protected]>--- block/ssh.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
PATCH v2 3/7 block/sheepdog: remove spurious NULL check
In reply to Jeff Cody'tag' is already checked in the lines immediately preceding this check, and set to non-NULL if NULL. No need to check again, it hasn't changed.
--- block/sheepdog.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
block/sheepdog.c +1 -1
@@ -1632,7 +1632,7 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,if (!tag) { tag ="" ; } -if (tag &&strlen (tag) >=SD_MAX_VDI_TAG_LEN ) { +if (strlen (tag) >=SD_MAX_VDI_TAG_LEN ) {error_setg (errp,"value of parameter 'tag' is too long" ); ret = -EINVAL ; goto err_no_fd;
Signature · 1 line
2.9.5
Re: [Qemu-devel] [PATCH v2 3/7] block/sheepdog: remove spurious NULL check
Quoted 13 lines · Jeff Cody
'tag' is already checked in the lines immediately preceding this check, and set to non-NULL if NULL. No need to check again, it hasn't changed.
Signed-off-by Jeff Cody <[email protected]>--- block/sheepdog.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)block/sheepdog.c +0
-0
block/sheepdog.c static int sd_open(BlockDriverState *bs, QDict *options, int flags, · line 1635
Quoted 5 lines
block/sheepdog.c +1
-1@@ -1632,7 +1632,7 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,if (!tag) { tag ="" ; } -if (tag &&strlen (tag) >=SD_MAX_VDI_TAG_LEN ) { +if (strlen (tag) >=SD_MAX_VDI_TAG_LEN ) {
you can even prepend an 'else'! :D
Quoted 7 lines
block/sheepdog.c +0
-1error_setg (errp,"value of parameter 'tag' is too long" ); ret = -EINVAL ; goto err_no_fd; --2.9.5
Re: [Qemu-devel] [PATCH v2 3/7] block/sheepdog: remove spurious NULL check
In reply to Jeff CodyQuoted 7 lines · Jeff Cody
'tag' is already checked in the lines immediately preceding this check, and set to non-NULL if NULL. No need to check again, it hasn't changed.
Signed-off-by Jeff Cody <[email protected]>--- block/sheepdog.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
PATCH v2 4/7 block/sheepdog: code beautification
In reply to Jeff CodyNo functional changes, just whitespace manipulation.
--- block/sheepdog.c | 162 +++++++++++++++++++++++++++---------------------------- 1 file changed, 81 insertions(+), 81 deletions(-)
block/sheepdog.c +81 -81
@@ -400,7 +400,7 @@ typedef struct BDRVSheepdogReopenState {int cache_flags; } BDRVSheepdogReopenState; -static const char *sd_strerror (int err) +static const char *sd_strerror (int err) {int i;
@@ -3078,111 +3078,111 @@ static QemuOptsList sd_create_opts = { };static BlockDriver bdrv_sheepdog = { - .format_name ="sheepdog" , - .protocol_name ="sheepdog" , - .instance_size =sizeof (BDRVSheepdogState), - .bdrv_parse_filename = sd_parse_filename, - .bdrv_file_open = sd_open, - .bdrv_reopen_prepare = sd_reopen_prepare, - .bdrv_reopen_commit = sd_reopen_commit, - .bdrv_reopen_abort = sd_reopen_abort, - .bdrv_close = sd_close, - .bdrv_create = sd_create, - .bdrv_has_zero_init = bdrv_has_zero_init_1, - .bdrv_getlength = sd_getlength, + .format_name ="sheepdog" , + .protocol_name ="sheepdog" , + .instance_size =sizeof (BDRVSheepdogState), + .bdrv_parse_filename = sd_parse_filename, + .bdrv_file_open = sd_open, + .bdrv_reopen_prepare = sd_reopen_prepare, + .bdrv_reopen_commit = sd_reopen_commit, + .bdrv_reopen_abort = sd_reopen_abort, + .bdrv_close = sd_close, + .bdrv_create = sd_create, + .bdrv_has_zero_init = bdrv_has_zero_init_1, + .bdrv_getlength = sd_getlength, .bdrv_get_allocated_file_size = sd_get_allocated_file_size, - .bdrv_truncate = sd_truncate, + .bdrv_truncate = sd_truncate, - .bdrv_co_readv = sd_co_readv, - .bdrv_co_writev = sd_co_writev, - .bdrv_co_flush_to_disk = sd_co_flush_to_disk, - .bdrv_co_pdiscard = sd_co_pdiscard, - .bdrv_co_get_block_status = sd_co_get_block_status, + .bdrv_co_readv = sd_co_readv, + .bdrv_co_writev = sd_co_writev, + .bdrv_co_flush_to_disk = sd_co_flush_to_disk, + .bdrv_co_pdiscard = sd_co_pdiscard, + .bdrv_co_get_block_status = sd_co_get_block_status, - .bdrv_snapshot_create = sd_snapshot_create, - .bdrv_snapshot_goto = sd_snapshot_goto, - .bdrv_snapshot_delete = sd_snapshot_delete, - .bdrv_snapshot_list = sd_snapshot_list, + .bdrv_snapshot_create = sd_snapshot_create, + .bdrv_snapshot_goto = sd_snapshot_goto, + .bdrv_snapshot_delete = sd_snapshot_delete, + .bdrv_snapshot_list = sd_snapshot_list, - .bdrv_save_vmstate = sd_save_vmstate, - .bdrv_load_vmstate = sd_load_vmstate, + .bdrv_save_vmstate = sd_save_vmstate, + .bdrv_load_vmstate = sd_load_vmstate, - .bdrv_detach_aio_context = sd_detach_aio_context, - .bdrv_attach_aio_context = sd_attach_aio_context, + .bdrv_detach_aio_context = sd_detach_aio_context, + .bdrv_attach_aio_context = sd_attach_aio_context, - .create_opts = &sd_create_opts, + .create_opts = &sd_create_opts, };static BlockDriver bdrv_sheepdog_tcp = { - .format_name ="sheepdog" , - .protocol_name ="sheepdog+tcp" , - .instance_size =sizeof (BDRVSheepdogState), - .bdrv_parse_filename = sd_parse_filename, + .format_name ="sheepdog" , + .protocol_name ="sheepdog+tcp" , + .instance_size =sizeof (BDRVSheepdogState), + .bdrv_parse_filename = sd_parse_filename, .bdrv_file_open = sd_open, - .bdrv_reopen_prepare = sd_reopen_prepare, - .bdrv_reopen_commit = sd_reopen_commit, - .bdrv_reopen_abort = sd_reopen_abort, - .bdrv_close = sd_close, - .bdrv_create = sd_create, - .bdrv_has_zero_init = bdrv_has_zero_init_1, - .bdrv_getlength = sd_getlength, + .bdrv_reopen_prepare = sd_reopen_prepare, + .bdrv_reopen_commit = sd_reopen_commit, + .bdrv_reopen_abort = sd_reopen_abort, + .bdrv_close = sd_close, + .bdrv_create = sd_create, + .bdrv_has_zero_init = bdrv_has_zero_init_1, + .bdrv_getlength = sd_getlength, .bdrv_get_allocated_file_size = sd_get_allocated_file_size, - .bdrv_truncate = sd_truncate, + .bdrv_truncate = sd_truncate, - .bdrv_co_readv = sd_co_readv, - .bdrv_co_writev = sd_co_writev, - .bdrv_co_flush_to_disk = sd_co_flush_to_disk, - .bdrv_co_pdiscard = sd_co_pdiscard, - .bdrv_co_get_block_status = sd_co_get_block_status, + .bdrv_co_readv = sd_co_readv, + .bdrv_co_writev = sd_co_writev, + .bdrv_co_flush_to_disk = sd_co_flush_to_disk, + .bdrv_co_pdiscard = sd_co_pdiscard, + .bdrv_co_get_block_status = sd_co_get_block_status, - .bdrv_snapshot_create = sd_snapshot_create, - .bdrv_snapshot_goto = sd_snapshot_goto, - .bdrv_snapshot_delete = sd_snapshot_delete, - .bdrv_snapshot_list = sd_snapshot_list, + .bdrv_snapshot_create = sd_snapshot_create, + .bdrv_snapshot_goto = sd_snapshot_goto, + .bdrv_snapshot_delete = sd_snapshot_delete, + .bdrv_snapshot_list = sd_snapshot_list, - .bdrv_save_vmstate = sd_save_vmstate, - .bdrv_load_vmstate = sd_load_vmstate, + .bdrv_save_vmstate = sd_save_vmstate, + .bdrv_load_vmstate = sd_load_vmstate, - .bdrv_detach_aio_context = sd_detach_aio_context, - .bdrv_attach_aio_context = sd_attach_aio_context, + .bdrv_detach_aio_context = sd_detach_aio_context, + .bdrv_attach_aio_context = sd_attach_aio_context, - .create_opts = &sd_create_opts, + .create_opts = &sd_create_opts, };static BlockDriver bdrv_sheepdog_unix = { - .format_name ="sheepdog" , - .protocol_name ="sheepdog+unix" , - .instance_size =sizeof (BDRVSheepdogState), - .bdrv_parse_filename = sd_parse_filename, - .bdrv_file_open = sd_open, - .bdrv_reopen_prepare = sd_reopen_prepare, - .bdrv_reopen_commit = sd_reopen_commit, - .bdrv_reopen_abort = sd_reopen_abort, - .bdrv_close = sd_close, - .bdrv_create = sd_create, - .bdrv_has_zero_init = bdrv_has_zero_init_1, - .bdrv_getlength = sd_getlength, + .format_name ="sheepdog" , + .protocol_name ="sheepdog+unix" , + .instance_size =sizeof (BDRVSheepdogState), + .bdrv_parse_filename = sd_parse_filename, + .bdrv_file_open = sd_open, + .bdrv_reopen_prepare = sd_reopen_prepare, + .bdrv_reopen_commit = sd_reopen_commit, + .bdrv_reopen_abort = sd_reopen_abort, + .bdrv_close = sd_close, + .bdrv_create = sd_create, + .bdrv_has_zero_init = bdrv_has_zero_init_1, + .bdrv_getlength = sd_getlength, .bdrv_get_allocated_file_size = sd_get_allocated_file_size, - .bdrv_truncate = sd_truncate, + .bdrv_truncate = sd_truncate, - .bdrv_co_readv = sd_co_readv, - .bdrv_co_writev = sd_co_writev, - .bdrv_co_flush_to_disk = sd_co_flush_to_disk, - .bdrv_co_pdiscard = sd_co_pdiscard, - .bdrv_co_get_block_status = sd_co_get_block_status, + .bdrv_co_readv = sd_co_readv, + .bdrv_co_writev = sd_co_writev, + .bdrv_co_flush_to_disk = sd_co_flush_to_disk, + .bdrv_co_pdiscard = sd_co_pdiscard, + .bdrv_co_get_block_status = sd_co_get_block_status, - .bdrv_snapshot_create = sd_snapshot_create, - .bdrv_snapshot_goto = sd_snapshot_goto, - .bdrv_snapshot_delete = sd_snapshot_delete, - .bdrv_snapshot_list = sd_snapshot_list, + .bdrv_snapshot_create = sd_snapshot_create, + .bdrv_snapshot_goto = sd_snapshot_goto, + .bdrv_snapshot_delete = sd_snapshot_delete, + .bdrv_snapshot_list = sd_snapshot_list, - .bdrv_save_vmstate = sd_save_vmstate, - .bdrv_load_vmstate = sd_load_vmstate, + .bdrv_save_vmstate = sd_save_vmstate, + .bdrv_load_vmstate = sd_load_vmstate, - .bdrv_detach_aio_context = sd_detach_aio_context, - .bdrv_attach_aio_context = sd_attach_aio_context, + .bdrv_detach_aio_context = sd_detach_aio_context, + .bdrv_attach_aio_context = sd_attach_aio_context, - .create_opts = &sd_create_opts, + .create_opts = &sd_create_opts, };static void bdrv_sheepdog_init (void )
Signature · 1 line
2.9.5
Re: [Qemu-devel] [PATCH v2 4/7] block/sheepdog: code beautification
Quoted 11 lines · Jeff Cody
No functional changes, just whitespace manipulation.
Signed-off-by Jeff Cody <[email protected]>--- block/sheepdog.c | 162 +++++++++++++++++++++++++++---------------------------- 1 file changed, 81 insertions(+), 81 deletions(-)static BlockDriver
bdrv_sheepdog= {block/sheepdog.c +0
-2- .format_name = "sheepdog", - .protocol_name = "sheepdog",
block/sheepdog.c static QemuOptsList sd_create_opts = { · line 3092
Quoted 10 lines
block/sheepdog.c +0
-10- .instance_size = sizeof(BDRVSheepdogState), - .bdrv_parse_filename = sd_parse_filename, - .bdrv_file_open = sd_open, - .bdrv_reopen_prepare = sd_reopen_prepare, - .bdrv_reopen_commit = sd_reopen_commit, - .bdrv_reopen_abort = sd_reopen_abort, - .bdrv_close = sd_close, - .bdrv_create = sd_create, - .bdrv_has_zero_init = bdrv_has_zero_init_1, - .bdrv_getlength = sd_getlength,
The existing style is indeed ugly since it has no consistency,...
block/sheepdog.c static QemuOptsList sd_create_opts = { · line 3093
Quoted 13 lines
block/sheepdog.c +12
-0+ .format_name = "sheepdog", + .protocol_name = "sheepdog", + .instance_size = sizeof(BDRVSheepdogState), + .bdrv_parse_filename = sd_parse_filename, + .bdrv_file_open = sd_open, + .bdrv_reopen_prepare = sd_reopen_prepare, + .bdrv_reopen_commit = sd_reopen_commit, + .bdrv_reopen_abort = sd_reopen_abort, + .bdrv_close = sd_close, + .bdrv_create = sd_create, + .bdrv_has_zero_init = bdrv_has_zero_init_1, + .bdrv_getlength = sd_getlength, .bdrv_get_allocated_file_size = sd_get_allocated_file_size,
...but aligning '=' requires mass reformatting if you get any longer .bdrv_ function callback added down the road. If it were me, I'd just consistently use a single space everywhere, as in:
.format_name = "sheepdog",
.protocol_name = "sheepdog",
but that's my personal opinion, and not a hard rule, so I won't be bothered if you don't take it.
block/sheepdog.c static QemuOptsList sd_create_opts = { · line 3121
Quoted 10 lines
static BlockDriver
bdrv_sheepdog_tcp= {block/sheepdog.c +4
-4- .format_name = "sheepdog", - .protocol_name = "sheepdog+tcp", - .instance_size = sizeof(BDRVSheepdogState), - .bdrv_parse_filename = sd_parse_filename, + .format_name = "sheepdog", + .protocol_name = "sheepdog+tcp", + .instance_size = sizeof(BDRVSheepdogState), + .bdrv_parse_filename = sd_parse_filename, .bdrv_file_open = sd_open,
See, with your style, this row now stands out as being missed; with my style, this line needs no edits.
But your changes are indeed semantic no-ops, so whether you fix the few unaligned lines per your style, or redo to a consistent style of only a single space, you can add:
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
PATCH v2 5/7 block/curl: check error return of curl_global_init()
In reply to Jeff CodyIf curl_global_init() fails, per the documentation no other curl functions may be called, so make sure to check the return value.
Also, some minor changes to the initialization latch variable 'inited':
- Make it static in the file, for clarity
- Change the name for clarity
- Make it a bool
--- block/curl.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
block/curl.c +12 -6
@@ -89,6 +89,8 @@ static CURLMcode __curl_multi_socket_action(CURLM *multi_handle,struct BDRVCURLState ; +static bool libcurl_initialized; + typedef struct CURLAIOCB { Coroutine *co; QEMUIOVector *qiov;
@@ -686,14 +688,23 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,double d;const char *secretid;const char *protocol_delimiter; +int ret; -static int inited =0 ;if (flags &BDRV_O_RDWR ) {error_setg (errp,"curl block device does not support writes" );return -EROFS ; } +if (!libcurl_initialized) { + ret =curl_global_init (CURL_GLOBAL_ALL ); +if (ret) { +error_setg (errp,"libcurl initialization failed with %d" , ret); +return -EIO ; + } + libcurl_initialized = true; + } +qemu_mutex_init (&s->mutex); opts =qemu_opts_create (&runtime_opts,NULL ,0 , &error_abort);qemu_opts_absorb_qdict (opts, options, &local_err);
@@ -772,11 +783,6 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags, } } -if (!inited) { -curl_global_init (CURL_GLOBAL_ALL ); - inited =1 ; - } -DPRINTF ("CURL: Opening %s\n" , file);QSIMPLEQ_INIT (&s->free_state_waitq); s->aio_context =bdrv_get_aio_context (bs);
Signature · 1 line
2.9.5
Re: [Qemu-devel] [PATCH v2 5/7] block/curl: check error return of curl_global_init()
block/curl.c
Quoted 15 lines · Jeff Cody
If
curl_global_init()fails, per the documentation no other curl functions may be called, so make sure to check the return value.Also, some minor changes to the initialization latch variable 'inited':
- Make it static in the file, for clarity
- Change the name for clarity
- Make it a boolSigned-off-by Jeff Cody <[email protected]>--- block/curl.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)block/curl.c +0
-0
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
PATCH v2 6/7 block/curl: fix minor memory leaks
In reply to Jeff Cody--- block/curl.c | 6 ++++++ 1 file changed, 6 insertions(+)
block/curl.c +6 -0
@@ -857,6 +857,9 @@ out_noclean:qemu_mutex_destroy (&s->mutex);g_free (s->cookie);g_free (s->url); +g_free (s->username); +g_free (s->proxyusername); +g_free (s->proxypassword);qemu_opts_del (opts);return -EINVAL ; }
@@ -955,6 +958,9 @@ static void curl_close(BlockDriverState *bs)g_free (s->cookie);g_free (s->url); +g_free (s->username); +g_free (s->proxyusername); +g_free (s->proxypassword); }static int64_t curl_getlength (BlockDriverState *bs)
Signature · 1 line
2.9.5
Re: [Qemu-devel] [PATCH v2 6/7] block/curl: fix minor memory leaks
Quoted 8 lines · Jeff Cody
Signed-off-by Jeff Cody <[email protected]>--- block/curl.c | 6 ++++++ 1 file changed, 6 insertions(+)+++
b/block/curl.c
block/curl.c out_noclean: · line 862
Quoted 6 lines
block/curl.c +3
-0@@ -857,6 +857,9 @@ out_noclean:qemu_mutex_destroy (&s->mutex);g_free (s->cookie);g_free (s->url); +g_free (s->username); +g_free (s->proxyusername); +g_free (s->proxypassword);
Would it be any simpler to call curl_close(s) instead of open-coding it here in this cleanup path?
block/curl.c static void curl_close(BlockDriverState *bs) · line 964
Quoted 8 lines
block/curl.c +3
-0@@ -955,6 +958,9 @@ static void curl_close(BlockDriverState *bs)g_free (s->cookie);g_free (s->url); +g_free (s->username); +g_free (s->proxyusername); +g_free (s->proxypassword); }
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
PATCH v2 7/7 block/curl: code cleanup to comply with coding style
In reply to Jeff CodyThis addresses non-functional changes to help curl.c better comply with the coding styles (comments, indentation, brackets, etc.).
One minor code change is the combination of two if statements into a single if statement.
--- block/curl.c | 100 +++++++++++++++++++++++++++++++---------------------------- 1 file changed, 52 insertions(+), 48 deletions(-)
block/curl.c +52 -48
@@ -32,8 +32,10 @@#include <curl/curl.h> #include "qemu/cutils.h" -// #define DEBUG_CURL -// #define DEBUG_VERBOSE +/* +#define DEBUG_CURL +#define DEBUG_VERBOSE +*/ #ifdef DEBUG_CURL #define DEBUG_CURL_PRINT 1
@@ -76,15 +78,15 @@ static CURLMcode __curl_multi_socket_action(CURLM *multi_handle,#define CURL_TIMEOUT_DEFAULT 5#define CURL_TIMEOUT_MAX 10000 -#define CURL_BLOCK_OPT_URL "url" -#define CURL_BLOCK_OPT_READAHEAD "readahead" -#define CURL_BLOCK_OPT_SSLVERIFY "sslverify" -#define CURL_BLOCK_OPT_TIMEOUT "timeout" -#define CURL_BLOCK_OPT_COOKIE "cookie" -#define CURL_BLOCK_OPT_COOKIE_SECRET "cookie-secret" -#define CURL_BLOCK_OPT_USERNAME "username" -#define CURL_BLOCK_OPT_PASSWORD_SECRET "password-secret" -#define CURL_BLOCK_OPT_PROXY_USERNAME "proxy-username" +#define CURL_BLOCK_OPT_URL "url" +#define CURL_BLOCK_OPT_READAHEAD "readahead" +#define CURL_BLOCK_OPT_SSLVERIFY "sslverify" +#define CURL_BLOCK_OPT_TIMEOUT "timeout" +#define CURL_BLOCK_OPT_COOKIE "cookie" +#define CURL_BLOCK_OPT_COOKIE_SECRET "cookie-secret" +#define CURL_BLOCK_OPT_USERNAME "username" +#define CURL_BLOCK_OPT_PASSWORD_SECRET "password-secret" +#define CURL_BLOCK_OPT_PROXY_USERNAME "proxy-username"#define CURL_BLOCK_OPT_PROXY_PASSWORD_SECRET "proxy-password-secret"struct BDRVCURLState ;
@@ -110,8 +112,7 @@ typedef struct CURLSocket {QLIST_ENTRY (CURLSocket) next; } CURLSocket; -typedef struct CURLState -{ +typedef struct CURLState { struct BDRVCURLState *s; CURLAIOCB *acb[CURL_NUM_ACB]; CURL *curl;
@@ -196,22 +197,22 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,DPRINTF ("CURL (AIO): Sock action %d on fd %d\n" , action, (int )fd);switch (action) { -case CURL_POLL_IN : -aio_set_fd_handler (s->aio_context, fd, false, - curl_multi_read,NULL ,NULL , state); -break ; -case CURL_POLL_OUT : -aio_set_fd_handler (s->aio_context, fd, false, -NULL , curl_multi_do,NULL , state); -break ; -case CURL_POLL_INOUT : -aio_set_fd_handler (s->aio_context, fd, false, - curl_multi_read, curl_multi_do,NULL , state); -break ; -case CURL_POLL_REMOVE : -aio_set_fd_handler (s->aio_context, fd, false, -NULL ,NULL ,NULL ,NULL ); -break ; +case CURL_POLL_IN : +aio_set_fd_handler (s->aio_context, fd, false, + curl_multi_read,NULL ,NULL , state); +break ; +case CURL_POLL_OUT : +aio_set_fd_handler (s->aio_context, fd, false, +NULL , curl_multi_do,NULL , state); +break ; +case CURL_POLL_INOUT : +aio_set_fd_handler (s->aio_context, fd, false, + curl_multi_read, curl_multi_do,NULL , state); +break ; +case CURL_POLL_REMOVE : +aio_set_fd_handler (s->aio_context, fd, false, +NULL ,NULL ,NULL ,NULL ); +break ; }return 0 ;
@@ -235,7 +236,7 @@ static size_t curl_header_cb(void *ptr, size_t size, size_t nmemb, void *opaque)/* Called from curl_multi_do_locked, with s->mutex held. */ static size_t curl_read_cb (void *ptr,size_t size,size_t nmemb,void *opaque) { -CURLState *s = ((CURLState *)opaque); +CURLState *s = ((CURLState *)opaque);size_t realsize = size * nmemb;int i;
@@ -253,11 +254,12 @@ static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void *opaque)memcpy (s->orig_buf + s->buf_off, ptr, realsize); s->buf_off += realsize; -for (i=0 ; i<CURL_NUM_ACB ; i++) { +for (i =0 ; i <CURL_NUM_ACB ; i++) {CURLAIOCB *acb = s->acb[i]; -if (!acb) +if (!acb) {continue ; + }if ((s->buf_off >= acb->end)) {size_t request_length = acb->bytes;
@@ -293,17 +295,16 @@ static bool curl_find_buf(BDRVCURLState *s, uint64_t start, uint64_t len,uint64_t clamped_end =MIN (end, s->len);uint64_t clamped_len = clamped_end - start; -for (i=0 ; i<CURL_NUM_STATES ; i++) { +for (i =0 ; i <CURL_NUM_STATES ; i++) {CURLState *state = &s->states[i];uint64_t buf_end = (state->buf_start + state->buf_off);uint64_t buf_fend = (state->buf_start + state->buf_len); -if (!state->orig_buf) -continue ; -if (!state->buf_off) +if (!state->orig_buf || !state->buf_off) {continue ; + } -// Does the existing buffer cover our section? +/* Does the existing buffer cover our section? */ if ((start >= state->buf_start) && (start <= buf_end) && (clamped_end >= state->buf_start) &&
@@ -319,7 +320,7 @@ static bool curl_find_buf(BDRVCURLState *s, uint64_t start, uint64_t len,return true; } -// Wait for unfinished chunks +/* Wait for unfinished chunks */ if (state->in_use && (start >= state->buf_start) && (start <= buf_fend) &&
@@ -331,7 +332,7 @@ static bool curl_find_buf(BDRVCURLState *s, uint64_t start, uint64_t len, acb->start = start - state->buf_start; acb->end = acb->start + clamped_len; -for (j=0 ; j<CURL_NUM_ACB ; j++) { +for (j =0 ; j <CURL_NUM_ACB ; j++) {if (!state->acb[j]) { state->acb[j] = acb;return true;
@@ -355,8 +356,9 @@ static void curl_multi_check_completion(BDRVCURLState *s) msg =curl_multi_info_read (s->multi, &msgs_in_queue);/* Quit when there are no more completions */ -if (!msg) +if (!msg) {break ; + }if (msg->msg ==CURLMSG_DONE ) {CURLState *state =NULL ;
@@ -540,12 +542,14 @@ static void curl_clean_state(CURLState *s) {CURLAIOCB *next;int j; +for (j =0 ; j <CURL_NUM_ACB ; j++) {assert (!s->acb[j]); } -if (s->s->multi) +if (s->s->multi) {curl_multi_remove_handle (s->s->multi, s->curl); + }while (!QLIST_EMPTY (&s->sockets)) {CURLSocket *socket =QLIST_FIRST (&s->sockets);
@@ -794,7 +798,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags, goto out_noclean; } -// Get file size +/* Get file size */ if (curl_init_state (s, state) <0 ) { goto out;
@@ -802,11 +806,11 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags, s->accept_range = false;curl_easy_setopt (state->curl,CURLOPT_NOBODY ,1 ); -curl_easy_setopt (state->curl,CURLOPT_HEADERFUNCTION , - curl_header_cb); +curl_easy_setopt (state->curl,CURLOPT_HEADERFUNCTION , curl_header_cb);curl_easy_setopt (state->curl,CURLOPT_HEADERDATA , s); -if (curl_easy_perform (state->curl)) +if (curl_easy_perform (state->curl)) { goto out; + }if (curl_easy_getinfo (state->curl,CURLINFO_CONTENT_LENGTH_DOWNLOAD , &d)) { goto out; }
@@ -876,13 +880,13 @@ static void curl_setup_preadv(BlockDriverState *bs, CURLAIOCB *acb)qemu_mutex_lock (&s->mutex); -// In case we have the requested data already (e.g. read-ahead), -// we can just call the callback and be done. +/* In case we have the requested data already (e.g. read-ahead), +we can just call the callback and be done. */ if (curl_find_buf (s, start, acb->bytes, acb)) { goto out; } -// No cache found, so let's start a new request +/* No cache found, so let's start a new request */ for (;;) { state =curl_find_state (s);if (state) {
Signature · 1 line
2.9.5
Re: [Qemu-devel] [PATCH v2 7/7] block/curl: code cleanup to comply with coding style
Quoted 12 lines · Jeff Cody
This addresses non-functional changes to help
curl.cbetter comply with the coding styles (comments, indentation, brackets, etc.).One minor code change is the combination of two if statements into a single if statement.
Signed-off-by Jeff Cody <[email protected]>--- block/curl.c | 100 +++++++++++++++++++++++++++++++---------------------------- 1 file changed, 52 insertions(+), 48 deletions(-)
block/curl.c static CURLMcode __curl_multi_socket_action(CURLM *multi_handle, · line 87
Quoted 9 lines
block/curl.c +0
-9-#define CURL_BLOCK_OPT_URL "url" -#define CURL_BLOCK_OPT_READAHEAD "readahead" -#define CURL_BLOCK_OPT_SSLVERIFY "sslverify" -#define CURL_BLOCK_OPT_TIMEOUT "timeout" -#define CURL_BLOCK_OPT_COOKIE "cookie" -#define CURL_BLOCK_OPT_COOKIE_SECRET "cookie-secret" -#define CURL_BLOCK_OPT_USERNAME "username" -#define CURL_BLOCK_OPT_PASSWORD_SECRET "password-secret" -#define CURL_BLOCK_OPT_PROXY_USERNAME "proxy-username"
Another change from inconsistent spacing,
block/curl.c static CURLMcode __curl_multi_socket_action(CURLM *multi_handle, · line 90
Quoted 10 lines
block/curl.c +9
-0+#define CURL_BLOCK_OPT_URL "url" +#define CURL_BLOCK_OPT_READAHEAD "readahead" +#define CURL_BLOCK_OPT_SSLVERIFY "sslverify" +#define CURL_BLOCK_OPT_TIMEOUT "timeout" +#define CURL_BLOCK_OPT_COOKIE "cookie" +#define CURL_BLOCK_OPT_COOKIE_SECRET "cookie-secret" +#define CURL_BLOCK_OPT_USERNAME "username" +#define CURL_BLOCK_OPT_PASSWORD_SECRET "password-secret" +#define CURL_BLOCK_OPT_PROXY_USERNAME "proxy-username"#define CURL_BLOCK_OPT_PROXY_PASSWORD_SECRET "proxy-password-secret"
to something that is consistent but requires a mass-edit if we add a longer name.
block/curl.c static size_t curl_header_cb(void *ptr, size_t size, size_t nmemb, void *opaque) · line 239
Quoted 6 lines
block/curl.c +1
-1@@ -235,7 +236,7 @@ static size_t curl_header_cb(void *ptr, size_t size, size_t nmemb, void *opaque)/* Called from curl_multi_do_locked, with s->mutex held. */ static size_t curl_read_cb (void *ptr,size_t size,size_t nmemb,void *opaque) { -CURLState *s = ((CURLState *)opaque); +CURLState *s = ((CURLState *)opaque);
Umm, this is C, so why not just:
CURLState *s = opaque;
Those are minor findings, so whether or not you address them,
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org