* [Qemu-devel] [PATCH 01/20] nbd: produce a better error if neither host nor port is passed
2014-02-09 9:48 [Qemu-devel] [PATCH 00/20] Improve bdrv_open error messages Paolo Bonzini
@ 2014-02-09 9:48 ` Paolo Bonzini
2014-02-10 7:27 ` Fam Zheng
2014-02-09 9:48 ` [Qemu-devel] [PATCH 02/20] nbd: correctly propagate errors Paolo Bonzini
` (18 subsequent siblings)
19 siblings, 1 reply; 48+ messages in thread
From: Paolo Bonzini @ 2014-02-09 9:48 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, mreitz
Before:
$ qemu-io-old
qemu-io-old> open -r -o file.driver=nbd
qemu-io-old: can't open device (null): Could not open image: Invalid argument
$ ./qemu-io-old
qemu-io-old> open -r -o file.driver=nbd,file.host=foo,file.path=bar
path and host may not be used at the same time.
qemu-io-old: can't open device (null): Could not open image: Invalid argument
After:
$ ./qemu-io
qemu-io> open -r -o file.driver=nbd
one of path and host must be specified.
qemu-io: can't open device (null): Could not open image: Invalid argument
$ ./qemu-io
qemu-io> open -r -o file.driver=nbd,file.host=foo,file.path=bar
path and host may not be used at the same time.
qemu-io: can't open device (null): Could not open image: Invalid argument
Next patch will fix the error propagation.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/nbd.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/block/nbd.c b/block/nbd.c
index 327e913..fd89083 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -192,19 +192,18 @@ static int nbd_config(BDRVNBDState *s, QDict *options, char **export)
{
Error *local_err = NULL;
- if (qdict_haskey(options, "path")) {
- if (qdict_haskey(options, "host")) {
+ if (qdict_haskey(options, "path") == qdict_haskey(options, "host")) {
+ if (qdict_haskey(options, "path")) {
qerror_report(ERROR_CLASS_GENERIC_ERROR, "path and host may not "
"be used at the same time.");
- return -EINVAL;
+ } else {
+ qerror_report(ERROR_CLASS_GENERIC_ERROR, "one of path and host "
+ "must be specified.");
}
- s->client.is_unix = true;
- } else if (qdict_haskey(options, "host")) {
- s->client.is_unix = false;
- } else {
return -EINVAL;
}
+ s->client.is_unix = qdict_haskey(options, "path");
s->socket_opts = qemu_opts_create(&socket_optslist, NULL, 0,
&error_abort);
--
1.8.5.3
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [Qemu-devel] [PATCH 01/20] nbd: produce a better error if neither host nor port is passed
2014-02-09 9:48 ` [Qemu-devel] [PATCH 01/20] nbd: produce a better error if neither host nor port is passed Paolo Bonzini
@ 2014-02-10 7:27 ` Fam Zheng
0 siblings, 0 replies; 48+ messages in thread
From: Fam Zheng @ 2014-02-10 7:27 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, qemu-devel, stefanha, mreitz
On Sun, 02/09 10:48, Paolo Bonzini wrote:
> Before:
> $ qemu-io-old
> qemu-io-old> open -r -o file.driver=nbd
> qemu-io-old: can't open device (null): Could not open image: Invalid argument
> $ ./qemu-io-old
> qemu-io-old> open -r -o file.driver=nbd,file.host=foo,file.path=bar
> path and host may not be used at the same time.
> qemu-io-old: can't open device (null): Could not open image: Invalid argument
>
> After:
> $ ./qemu-io
> qemu-io> open -r -o file.driver=nbd
> one of path and host must be specified.
> qemu-io: can't open device (null): Could not open image: Invalid argument
> $ ./qemu-io
> qemu-io> open -r -o file.driver=nbd,file.host=foo,file.path=bar
> path and host may not be used at the same time.
> qemu-io: can't open device (null): Could not open image: Invalid argument
>
> Next patch will fix the error propagation.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> block/nbd.c | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/block/nbd.c b/block/nbd.c
> index 327e913..fd89083 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -192,19 +192,18 @@ static int nbd_config(BDRVNBDState *s, QDict *options, char **export)
> {
> Error *local_err = NULL;
>
> - if (qdict_haskey(options, "path")) {
> - if (qdict_haskey(options, "host")) {
> + if (qdict_haskey(options, "path") == qdict_haskey(options, "host")) {
> + if (qdict_haskey(options, "path")) {
> qerror_report(ERROR_CLASS_GENERIC_ERROR, "path and host may not "
> "be used at the same time.");
> - return -EINVAL;
> + } else {
> + qerror_report(ERROR_CLASS_GENERIC_ERROR, "one of path and host "
> + "must be specified.");
> }
> - s->client.is_unix = true;
> - } else if (qdict_haskey(options, "host")) {
> - s->client.is_unix = false;
> - } else {
> return -EINVAL;
> }
>
> + s->client.is_unix = qdict_haskey(options, "path");
> s->socket_opts = qemu_opts_create(&socket_optslist, NULL, 0,
> &error_abort);
>
> --
> 1.8.5.3
>
>
>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 48+ messages in thread
* [Qemu-devel] [PATCH 02/20] nbd: correctly propagate errors
2014-02-09 9:48 [Qemu-devel] [PATCH 00/20] Improve bdrv_open error messages Paolo Bonzini
2014-02-09 9:48 ` [Qemu-devel] [PATCH 01/20] nbd: produce a better error if neither host nor port is passed Paolo Bonzini
@ 2014-02-09 9:48 ` Paolo Bonzini
2014-02-10 7:38 ` Fam Zheng
2014-02-09 9:48 ` [Qemu-devel] [PATCH 03/20] nbd: inline tcp_socket_incoming_spec into sole caller Paolo Bonzini
` (17 subsequent siblings)
19 siblings, 1 reply; 48+ messages in thread
From: Paolo Bonzini @ 2014-02-09 9:48 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, mreitz
Before:
$ ./qemu-io-old
qemu-io-old> open -r -o file.driver=nbd
one of path and host must be specified.
qemu-io-old: can't open device (null): Could not open image: Invalid argument
$ ./qemu-io-old
qemu-io-old> open -r -o file.driver=nbd,file.host=foo,file.path=bar
path and host may not be used at the same time.
qemu-io-old: can't open device (null): Could not open image: Invalid argument
After:
$ ./qemu-io
qemu-io> open -r -o file.driver=nbd
qemu-io: can't open device (null): one of path and host must be specified.
$ ./qemu-io
qemu-io> open -r -o file.driver=nbd,file.host=foo,file.path=bar
qemu-io: can't open device (null): path and host may not be used at the same time.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/nbd.c | 34 ++++++++++++++++------------------
include/block/nbd.h | 1 -
nbd.c | 12 ------------
3 files changed, 16 insertions(+), 31 deletions(-)
diff --git a/block/nbd.c b/block/nbd.c
index fd89083..69f336b 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -188,19 +188,18 @@ out:
g_free(file);
}
-static int nbd_config(BDRVNBDState *s, QDict *options, char **export)
+static void nbd_config(BDRVNBDState *s, QDict *options, char **export,
+ Error **errp)
{
Error *local_err = NULL;
if (qdict_haskey(options, "path") == qdict_haskey(options, "host")) {
if (qdict_haskey(options, "path")) {
- qerror_report(ERROR_CLASS_GENERIC_ERROR, "path and host may not "
- "be used at the same time.");
+ error_setg(errp, "path and host may not be used at the same time.");
} else {
- qerror_report(ERROR_CLASS_GENERIC_ERROR, "one of path and host "
- "must be specified.");
+ error_setg(errp, "one of path and host must be specified.");
}
- return -EINVAL;
+ return;
}
s->client.is_unix = qdict_haskey(options, "path");
@@ -209,9 +208,8 @@ static int nbd_config(BDRVNBDState *s, QDict *options, char **export)
qemu_opts_absorb_qdict(s->socket_opts, options, &local_err);
if (error_is_set(&local_err)) {
- qerror_report_err(local_err);
- error_free(local_err);
- return -EINVAL;
+ error_propagate(errp, local_err);
+ return;
}
if (!qemu_opt_get(s->socket_opts, "port")) {
@@ -222,19 +220,17 @@ static int nbd_config(BDRVNBDState *s, QDict *options, char **export)
if (*export) {
qdict_del(options, "export");
}
-
- return 0;
}
-static int nbd_establish_connection(BlockDriverState *bs)
+static int nbd_establish_connection(BlockDriverState *bs, Error **errp)
{
BDRVNBDState *s = bs->opaque;
int sock;
if (s->client.is_unix) {
- sock = unix_socket_outgoing(qemu_opt_get(s->socket_opts, "path"));
+ sock = unix_connect(qemu_opt_get(s->socket_opts, "path"), errp);
} else {
- sock = tcp_socket_outgoing_opts(s->socket_opts);
+ sock = inet_connect_opts(s->socket_opts, errp, NULL, NULL);
if (sock >= 0) {
socket_set_nodelay(sock);
}
@@ -255,17 +251,19 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
BDRVNBDState *s = bs->opaque;
char *export = NULL;
int result, sock;
+ Error *local_err = NULL;
/* Pop the config into our state object. Exit if invalid. */
- result = nbd_config(s, options, &export);
- if (result != 0) {
- return result;
+ nbd_config(s, options, &export, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return -EINVAL;
}
/* establish TCP connection, return error if it fails
* TODO: Configurable retry-until-timeout behaviour.
*/
- sock = nbd_establish_connection(bs);
+ sock = nbd_establish_connection(bs, errp);
if (sock < 0) {
return sock;
}
diff --git a/include/block/nbd.h b/include/block/nbd.h
index c90f5e4..e10ab82 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -64,7 +64,6 @@ enum {
ssize_t nbd_wr_sync(int fd, void *buffer, size_t size, bool do_read);
int tcp_socket_incoming(const char *address, uint16_t port);
int tcp_socket_incoming_spec(const char *address_and_port);
-int tcp_socket_outgoing_opts(QemuOpts *opts);
int unix_socket_outgoing(const char *path);
int unix_socket_incoming(const char *path);
diff --git a/nbd.c b/nbd.c
index 030f56b..17ca95b 100644
--- a/nbd.c
+++ b/nbd.c
@@ -199,18 +199,6 @@ static void combine_addr(char *buf, size_t len, const char* address,
}
}
-int tcp_socket_outgoing_opts(QemuOpts *opts)
-{
- Error *local_err = NULL;
- int fd = inet_connect_opts(opts, &local_err, NULL, NULL);
- if (local_err != NULL) {
- qerror_report_err(local_err);
- error_free(local_err);
- }
-
- return fd;
-}
-
int tcp_socket_incoming(const char *address, uint16_t port)
{
char address_and_port[128];
--
1.8.5.3
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [Qemu-devel] [PATCH 02/20] nbd: correctly propagate errors
2014-02-09 9:48 ` [Qemu-devel] [PATCH 02/20] nbd: correctly propagate errors Paolo Bonzini
@ 2014-02-10 7:38 ` Fam Zheng
2014-02-10 8:24 ` Paolo Bonzini
0 siblings, 1 reply; 48+ messages in thread
From: Fam Zheng @ 2014-02-10 7:38 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, qemu-devel, stefanha, mreitz
On Sun, 02/09 10:48, Paolo Bonzini wrote:
> Before:
> $ ./qemu-io-old
> qemu-io-old> open -r -o file.driver=nbd
> one of path and host must be specified.
> qemu-io-old: can't open device (null): Could not open image: Invalid argument
> $ ./qemu-io-old
> qemu-io-old> open -r -o file.driver=nbd,file.host=foo,file.path=bar
> path and host may not be used at the same time.
> qemu-io-old: can't open device (null): Could not open image: Invalid argument
>
> After:
> $ ./qemu-io
> qemu-io> open -r -o file.driver=nbd
> qemu-io: can't open device (null): one of path and host must be specified.
> $ ./qemu-io
> qemu-io> open -r -o file.driver=nbd,file.host=foo,file.path=bar
> qemu-io: can't open device (null): path and host may not be used at the same time.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> block/nbd.c | 34 ++++++++++++++++------------------
> include/block/nbd.h | 1 -
> nbd.c | 12 ------------
> 3 files changed, 16 insertions(+), 31 deletions(-)
>
> diff --git a/block/nbd.c b/block/nbd.c
> index fd89083..69f336b 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -188,19 +188,18 @@ out:
> g_free(file);
> }
>
> -static int nbd_config(BDRVNBDState *s, QDict *options, char **export)
> +static void nbd_config(BDRVNBDState *s, QDict *options, char **export,
> + Error **errp)
> {
> Error *local_err = NULL;
>
> if (qdict_haskey(options, "path") == qdict_haskey(options, "host")) {
> if (qdict_haskey(options, "path")) {
> - qerror_report(ERROR_CLASS_GENERIC_ERROR, "path and host may not "
> - "be used at the same time.");
> + error_setg(errp, "path and host may not be used at the same time.");
> } else {
> - qerror_report(ERROR_CLASS_GENERIC_ERROR, "one of path and host "
> - "must be specified.");
> + error_setg(errp, "one of path and host must be specified.");
> }
> - return -EINVAL;
> + return;
> }
>
> s->client.is_unix = qdict_haskey(options, "path");
> @@ -209,9 +208,8 @@ static int nbd_config(BDRVNBDState *s, QDict *options, char **export)
>
> qemu_opts_absorb_qdict(s->socket_opts, options, &local_err);
> if (error_is_set(&local_err)) {
> - qerror_report_err(local_err);
> - error_free(local_err);
> - return -EINVAL;
> + error_propagate(errp, local_err);
> + return;
> }
>
> if (!qemu_opt_get(s->socket_opts, "port")) {
> @@ -222,19 +220,17 @@ static int nbd_config(BDRVNBDState *s, QDict *options, char **export)
> if (*export) {
> qdict_del(options, "export");
> }
> -
> - return 0;
> }
>
> -static int nbd_establish_connection(BlockDriverState *bs)
> +static int nbd_establish_connection(BlockDriverState *bs, Error **errp)
> {
> BDRVNBDState *s = bs->opaque;
> int sock;
>
> if (s->client.is_unix) {
> - sock = unix_socket_outgoing(qemu_opt_get(s->socket_opts, "path"));
> + sock = unix_connect(qemu_opt_get(s->socket_opts, "path"), errp);
Why not use unix_connect_opts like below?
> } else {
> - sock = tcp_socket_outgoing_opts(s->socket_opts);
> + sock = inet_connect_opts(s->socket_opts, errp, NULL, NULL);
> if (sock >= 0) {
> socket_set_nodelay(sock);
> }
> @@ -255,17 +251,19 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
> BDRVNBDState *s = bs->opaque;
> char *export = NULL;
> int result, sock;
> + Error *local_err = NULL;
>
> /* Pop the config into our state object. Exit if invalid. */
> - result = nbd_config(s, options, &export);
> - if (result != 0) {
> - return result;
> + nbd_config(s, options, &export, &local_err);
> + if (local_err) {
Isn't error_is_set() better here?
> + error_propagate(errp, local_err);
> + return -EINVAL;
> }
>
> /* establish TCP connection, return error if it fails
> * TODO: Configurable retry-until-timeout behaviour.
> */
> - sock = nbd_establish_connection(bs);
> + sock = nbd_establish_connection(bs, errp);
> if (sock < 0) {
> return sock;
> }
Thanks,
Fam
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [Qemu-devel] [PATCH 02/20] nbd: correctly propagate errors
2014-02-10 7:38 ` Fam Zheng
@ 2014-02-10 8:24 ` Paolo Bonzini
2014-02-10 9:11 ` Fam Zheng
0 siblings, 1 reply; 48+ messages in thread
From: Paolo Bonzini @ 2014-02-10 8:24 UTC (permalink / raw)
To: Fam Zheng; +Cc: kwolf, qemu-devel, stefanha, mreitz
Il 10/02/2014 08:38, Fam Zheng ha scritto:
>> > if (s->client.is_unix) {
>> > - sock = unix_socket_outgoing(qemu_opt_get(s->socket_opts, "path"));
>> > + sock = unix_connect(qemu_opt_get(s->socket_opts, "path"), errp);
> Why not use unix_connect_opts like below?
Right!
>> > } else {
>> > - sock = tcp_socket_outgoing_opts(s->socket_opts);
>> > + sock = inet_connect_opts(s->socket_opts, errp, NULL, NULL);
>> > if (sock >= 0) {
>> > socket_set_nodelay(sock);
>> > }
>> > @@ -255,17 +251,19 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
>> > BDRVNBDState *s = bs->opaque;
>> > char *export = NULL;
>> > int result, sock;
>> > + Error *local_err = NULL;
>> >
>> > /* Pop the config into our state object. Exit if invalid. */
>> > - result = nbd_config(s, options, &export);
>> > - if (result != 0) {
>> > - return result;
>> > + nbd_config(s, options, &export, &local_err);
>> > + if (local_err) {
> Isn't error_is_set() better here?
No, error_is_set(&foo) is the same as foo != NULL.
So we use error_is_set only with Error**, which really should never
happen because you miss errors if the errp is NULL. :)
Paolo
>> > + error_propagate(errp, local_err);
>> > + return -EINVAL;
>> > }
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [Qemu-devel] [PATCH 02/20] nbd: correctly propagate errors
2014-02-10 8:24 ` Paolo Bonzini
@ 2014-02-10 9:11 ` Fam Zheng
0 siblings, 0 replies; 48+ messages in thread
From: Fam Zheng @ 2014-02-10 9:11 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, qemu-devel, stefanha, mreitz
On Mon, 02/10 09:24, Paolo Bonzini wrote:
> Il 10/02/2014 08:38, Fam Zheng ha scritto:
> >>> if (s->client.is_unix) {
> >>> - sock = unix_socket_outgoing(qemu_opt_get(s->socket_opts, "path"));
> >>> + sock = unix_connect(qemu_opt_get(s->socket_opts, "path"), errp);
> >Why not use unix_connect_opts like below?
>
> Right!
>
> >>> } else {
> >>> - sock = tcp_socket_outgoing_opts(s->socket_opts);
> >>> + sock = inet_connect_opts(s->socket_opts, errp, NULL, NULL);
> >>> if (sock >= 0) {
> >>> socket_set_nodelay(sock);
> >>> }
> >>> @@ -255,17 +251,19 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
> >>> BDRVNBDState *s = bs->opaque;
> >>> char *export = NULL;
> >>> int result, sock;
> >>> + Error *local_err = NULL;
> >>>
> >>> /* Pop the config into our state object. Exit if invalid. */
> >>> - result = nbd_config(s, options, &export);
> >>> - if (result != 0) {
> >>> - return result;
> >>> + nbd_config(s, options, &export, &local_err);
> >>> + if (local_err) {
> >Isn't error_is_set() better here?
>
> No, error_is_set(&foo) is the same as foo != NULL.
>
> So we use error_is_set only with Error**, which really should never happen
> because you miss errors if the errp is NULL. :)
>
Clear! Thanks for the explanation!
Fam
^ permalink raw reply [flat|nested] 48+ messages in thread
* [Qemu-devel] [PATCH 03/20] nbd: inline tcp_socket_incoming_spec into sole caller
2014-02-09 9:48 [Qemu-devel] [PATCH 00/20] Improve bdrv_open error messages Paolo Bonzini
2014-02-09 9:48 ` [Qemu-devel] [PATCH 01/20] nbd: produce a better error if neither host nor port is passed Paolo Bonzini
2014-02-09 9:48 ` [Qemu-devel] [PATCH 02/20] nbd: correctly propagate errors Paolo Bonzini
@ 2014-02-09 9:48 ` Paolo Bonzini
2014-02-10 7:40 ` Fam Zheng
2014-02-09 9:48 ` [Qemu-devel] [PATCH 04/20] nbd: move socket wrappers to qemu-nbd Paolo Bonzini
` (16 subsequent siblings)
19 siblings, 1 reply; 48+ messages in thread
From: Paolo Bonzini @ 2014-02-09 9:48 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, mreitz
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
include/block/nbd.h | 1 -
nbd.c | 8 ++------
2 files changed, 2 insertions(+), 7 deletions(-)
diff --git a/include/block/nbd.h b/include/block/nbd.h
index e10ab82..1b39c06 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -63,7 +63,6 @@ enum {
ssize_t nbd_wr_sync(int fd, void *buffer, size_t size, bool do_read);
int tcp_socket_incoming(const char *address, uint16_t port);
-int tcp_socket_incoming_spec(const char *address_and_port);
int unix_socket_outgoing(const char *path);
int unix_socket_incoming(const char *path);
diff --git a/nbd.c b/nbd.c
index 17ca95b..2fc1f1f 100644
--- a/nbd.c
+++ b/nbd.c
@@ -202,13 +202,9 @@ static void combine_addr(char *buf, size_t len, const char* address,
int tcp_socket_incoming(const char *address, uint16_t port)
{
char address_and_port[128];
- combine_addr(address_and_port, 128, address, port);
- return tcp_socket_incoming_spec(address_and_port);
-}
-
-int tcp_socket_incoming_spec(const char *address_and_port)
-{
Error *local_err = NULL;
+
+ combine_addr(address_and_port, 128, address, port);
int fd = inet_listen(address_and_port, NULL, 0, SOCK_STREAM, 0, &local_err);
if (local_err != NULL) {
--
1.8.5.3
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [Qemu-devel] [PATCH 03/20] nbd: inline tcp_socket_incoming_spec into sole caller
2014-02-09 9:48 ` [Qemu-devel] [PATCH 03/20] nbd: inline tcp_socket_incoming_spec into sole caller Paolo Bonzini
@ 2014-02-10 7:40 ` Fam Zheng
0 siblings, 0 replies; 48+ messages in thread
From: Fam Zheng @ 2014-02-10 7:40 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, qemu-devel, stefanha, mreitz
On Sun, 02/09 10:48, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> include/block/nbd.h | 1 -
> nbd.c | 8 ++------
> 2 files changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index e10ab82..1b39c06 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -63,7 +63,6 @@ enum {
>
> ssize_t nbd_wr_sync(int fd, void *buffer, size_t size, bool do_read);
> int tcp_socket_incoming(const char *address, uint16_t port);
> -int tcp_socket_incoming_spec(const char *address_and_port);
> int unix_socket_outgoing(const char *path);
> int unix_socket_incoming(const char *path);
>
> diff --git a/nbd.c b/nbd.c
> index 17ca95b..2fc1f1f 100644
> --- a/nbd.c
> +++ b/nbd.c
> @@ -202,13 +202,9 @@ static void combine_addr(char *buf, size_t len, const char* address,
> int tcp_socket_incoming(const char *address, uint16_t port)
> {
> char address_and_port[128];
> - combine_addr(address_and_port, 128, address, port);
> - return tcp_socket_incoming_spec(address_and_port);
> -}
> -
> -int tcp_socket_incoming_spec(const char *address_and_port)
> -{
> Error *local_err = NULL;
> +
> + combine_addr(address_and_port, 128, address, port);
> int fd = inet_listen(address_and_port, NULL, 0, SOCK_STREAM, 0, &local_err);
>
> if (local_err != NULL) {
> --
> 1.8.5.3
>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 48+ messages in thread
* [Qemu-devel] [PATCH 04/20] nbd: move socket wrappers to qemu-nbd
2014-02-09 9:48 [Qemu-devel] [PATCH 00/20] Improve bdrv_open error messages Paolo Bonzini
` (2 preceding siblings ...)
2014-02-09 9:48 ` [Qemu-devel] [PATCH 03/20] nbd: inline tcp_socket_incoming_spec into sole caller Paolo Bonzini
@ 2014-02-09 9:48 ` Paolo Bonzini
2014-02-10 7:44 ` Fam Zheng
2014-02-09 9:48 ` [Qemu-devel] [PATCH 05/20] iscsi: fix indentation Paolo Bonzini
` (15 subsequent siblings)
19 siblings, 1 reply; 48+ messages in thread
From: Paolo Bonzini @ 2014-02-09 9:48 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, mreitz
qemu-nbd is one of the few valid users of qerror_report_err. Move
the error-reporting socket wrappers there.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
include/block/nbd.h | 4 ----
nbd.c | 50 --------------------------------------------------
qemu-nbd.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 52 insertions(+), 54 deletions(-)
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 1b39c06..79502a0 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -62,10 +62,6 @@ enum {
#define NBD_MAX_BUFFER_SIZE (32 * 1024 * 1024)
ssize_t nbd_wr_sync(int fd, void *buffer, size_t size, bool do_read);
-int tcp_socket_incoming(const char *address, uint16_t port);
-int unix_socket_outgoing(const char *path);
-int unix_socket_incoming(const char *path);
-
int nbd_receive_negotiate(int csock, const char *name, uint32_t *flags,
off_t *size, size_t *blocksize);
int nbd_init(int fd, int csock, uint32_t flags, off_t size, size_t blocksize);
diff --git a/nbd.c b/nbd.c
index 2fc1f1f..e5084b6 100644
--- a/nbd.c
+++ b/nbd.c
@@ -188,56 +188,6 @@ static ssize_t write_sync(int fd, void *buffer, size_t size)
return ret;
}
-static void combine_addr(char *buf, size_t len, const char* address,
- uint16_t port)
-{
- /* If the address-part contains a colon, it's an IPv6 IP so needs [] */
- if (strstr(address, ":")) {
- snprintf(buf, len, "[%s]:%u", address, port);
- } else {
- snprintf(buf, len, "%s:%u", address, port);
- }
-}
-
-int tcp_socket_incoming(const char *address, uint16_t port)
-{
- char address_and_port[128];
- Error *local_err = NULL;
-
- combine_addr(address_and_port, 128, address, port);
- int fd = inet_listen(address_and_port, NULL, 0, SOCK_STREAM, 0, &local_err);
-
- if (local_err != NULL) {
- qerror_report_err(local_err);
- error_free(local_err);
- }
- return fd;
-}
-
-int unix_socket_incoming(const char *path)
-{
- Error *local_err = NULL;
- int fd = unix_listen(path, NULL, 0, &local_err);
-
- if (local_err != NULL) {
- qerror_report_err(local_err);
- error_free(local_err);
- }
- return fd;
-}
-
-int unix_socket_outgoing(const char *path)
-{
- Error *local_err = NULL;
- int fd = unix_connect(path, &local_err);
-
- if (local_err != NULL) {
- qerror_report_err(local_err);
- error_free(local_err);
- }
- return fd;
-}
-
/* Basic flow for negotiation
Server Client
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 136e8c9..8138435 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -20,6 +20,8 @@
#include "block/block.h"
#include "block/nbd.h"
#include "qemu/main-loop.h"
+#include "qemu/sockets.h"
+#include "qemu/error-report.h"
#include "block/snapshot.h"
#include <stdarg.h>
@@ -201,6 +203,56 @@ static void termsig_handler(int signum)
qemu_notify_event();
}
+static void combine_addr(char *buf, size_t len, const char* address,
+ uint16_t port)
+{
+ /* If the address-part contains a colon, it's an IPv6 IP so needs [] */
+ if (strstr(address, ":")) {
+ snprintf(buf, len, "[%s]:%u", address, port);
+ } else {
+ snprintf(buf, len, "%s:%u", address, port);
+ }
+}
+
+static int tcp_socket_incoming(const char *address, uint16_t port)
+{
+ char address_and_port[128];
+ Error *local_err = NULL;
+
+ combine_addr(address_and_port, 128, address, port);
+ int fd = inet_listen(address_and_port, NULL, 0, SOCK_STREAM, 0, &local_err);
+
+ if (local_err != NULL) {
+ qerror_report_err(local_err);
+ error_free(local_err);
+ }
+ return fd;
+}
+
+static int unix_socket_incoming(const char *path)
+{
+ Error *local_err = NULL;
+ int fd = unix_listen(path, NULL, 0, &local_err);
+
+ if (local_err != NULL) {
+ qerror_report_err(local_err);
+ error_free(local_err);
+ }
+ return fd;
+}
+
+static int unix_socket_outgoing(const char *path)
+{
+ Error *local_err = NULL;
+ int fd = unix_connect(path, &local_err);
+
+ if (local_err != NULL) {
+ qerror_report_err(local_err);
+ error_free(local_err);
+ }
+ return fd;
+}
+
static void *show_parts(void *arg)
{
char *device = arg;
--
1.8.5.3
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [Qemu-devel] [PATCH 04/20] nbd: move socket wrappers to qemu-nbd
2014-02-09 9:48 ` [Qemu-devel] [PATCH 04/20] nbd: move socket wrappers to qemu-nbd Paolo Bonzini
@ 2014-02-10 7:44 ` Fam Zheng
0 siblings, 0 replies; 48+ messages in thread
From: Fam Zheng @ 2014-02-10 7:44 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, qemu-devel, stefanha, mreitz
On Sun, 02/09 10:48, Paolo Bonzini wrote:
> qemu-nbd is one of the few valid users of qerror_report_err. Move
> the error-reporting socket wrappers there.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> include/block/nbd.h | 4 ----
> nbd.c | 50 --------------------------------------------------
> qemu-nbd.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 52 insertions(+), 54 deletions(-)
>
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 1b39c06..79502a0 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -62,10 +62,6 @@ enum {
> #define NBD_MAX_BUFFER_SIZE (32 * 1024 * 1024)
>
> ssize_t nbd_wr_sync(int fd, void *buffer, size_t size, bool do_read);
> -int tcp_socket_incoming(const char *address, uint16_t port);
> -int unix_socket_outgoing(const char *path);
> -int unix_socket_incoming(const char *path);
> -
> int nbd_receive_negotiate(int csock, const char *name, uint32_t *flags,
> off_t *size, size_t *blocksize);
> int nbd_init(int fd, int csock, uint32_t flags, off_t size, size_t blocksize);
> diff --git a/nbd.c b/nbd.c
> index 2fc1f1f..e5084b6 100644
> --- a/nbd.c
> +++ b/nbd.c
> @@ -188,56 +188,6 @@ static ssize_t write_sync(int fd, void *buffer, size_t size)
> return ret;
> }
>
> -static void combine_addr(char *buf, size_t len, const char* address,
> - uint16_t port)
> -{
> - /* If the address-part contains a colon, it's an IPv6 IP so needs [] */
> - if (strstr(address, ":")) {
> - snprintf(buf, len, "[%s]:%u", address, port);
> - } else {
> - snprintf(buf, len, "%s:%u", address, port);
> - }
> -}
> -
> -int tcp_socket_incoming(const char *address, uint16_t port)
> -{
> - char address_and_port[128];
> - Error *local_err = NULL;
> -
> - combine_addr(address_and_port, 128, address, port);
> - int fd = inet_listen(address_and_port, NULL, 0, SOCK_STREAM, 0, &local_err);
> -
> - if (local_err != NULL) {
> - qerror_report_err(local_err);
> - error_free(local_err);
> - }
> - return fd;
> -}
> -
> -int unix_socket_incoming(const char *path)
> -{
> - Error *local_err = NULL;
> - int fd = unix_listen(path, NULL, 0, &local_err);
> -
> - if (local_err != NULL) {
> - qerror_report_err(local_err);
> - error_free(local_err);
> - }
> - return fd;
> -}
> -
> -int unix_socket_outgoing(const char *path)
> -{
> - Error *local_err = NULL;
> - int fd = unix_connect(path, &local_err);
> -
> - if (local_err != NULL) {
> - qerror_report_err(local_err);
> - error_free(local_err);
> - }
> - return fd;
> -}
> -
> /* Basic flow for negotiation
>
> Server Client
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 136e8c9..8138435 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -20,6 +20,8 @@
> #include "block/block.h"
> #include "block/nbd.h"
> #include "qemu/main-loop.h"
> +#include "qemu/sockets.h"
> +#include "qemu/error-report.h"
> #include "block/snapshot.h"
>
> #include <stdarg.h>
> @@ -201,6 +203,56 @@ static void termsig_handler(int signum)
> qemu_notify_event();
> }
>
> +static void combine_addr(char *buf, size_t len, const char* address,
> + uint16_t port)
> +{
> + /* If the address-part contains a colon, it's an IPv6 IP so needs [] */
> + if (strstr(address, ":")) {
> + snprintf(buf, len, "[%s]:%u", address, port);
> + } else {
> + snprintf(buf, len, "%s:%u", address, port);
> + }
> +}
> +
> +static int tcp_socket_incoming(const char *address, uint16_t port)
> +{
> + char address_and_port[128];
> + Error *local_err = NULL;
> +
> + combine_addr(address_and_port, 128, address, port);
> + int fd = inet_listen(address_and_port, NULL, 0, SOCK_STREAM, 0, &local_err);
> +
> + if (local_err != NULL) {
> + qerror_report_err(local_err);
> + error_free(local_err);
> + }
> + return fd;
> +}
> +
> +static int unix_socket_incoming(const char *path)
> +{
> + Error *local_err = NULL;
> + int fd = unix_listen(path, NULL, 0, &local_err);
> +
> + if (local_err != NULL) {
> + qerror_report_err(local_err);
> + error_free(local_err);
> + }
> + return fd;
> +}
> +
> +static int unix_socket_outgoing(const char *path)
> +{
> + Error *local_err = NULL;
> + int fd = unix_connect(path, &local_err);
> +
> + if (local_err != NULL) {
> + qerror_report_err(local_err);
> + error_free(local_err);
> + }
> + return fd;
> +}
> +
> static void *show_parts(void *arg)
> {
> char *device = arg;
> --
> 1.8.5.3
>
>
>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 48+ messages in thread
* [Qemu-devel] [PATCH 05/20] iscsi: fix indentation
2014-02-09 9:48 [Qemu-devel] [PATCH 00/20] Improve bdrv_open error messages Paolo Bonzini
` (3 preceding siblings ...)
2014-02-09 9:48 ` [Qemu-devel] [PATCH 04/20] nbd: move socket wrappers to qemu-nbd Paolo Bonzini
@ 2014-02-09 9:48 ` Paolo Bonzini
2014-02-10 7:48 ` Fam Zheng
2014-02-09 9:48 ` [Qemu-devel] [PATCH 06/20] iscsi: correctly propagate errors in iscsi_open Paolo Bonzini
` (14 subsequent siblings)
19 siblings, 1 reply; 48+ messages in thread
From: Paolo Bonzini @ 2014-02-09 9:48 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, mreitz
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/iscsi.c | 45 +++++++++++++++++++++++----------------------
1 file changed, 23 insertions(+), 22 deletions(-)
diff --git a/block/iscsi.c b/block/iscsi.c
index 6f4af72..e654a57 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1065,35 +1065,36 @@ static QemuOptsList runtime_opts = {
},
};
-static struct scsi_task *iscsi_do_inquiry(struct iscsi_context *iscsi,
- int lun, int evpd, int pc) {
- int full_size;
- struct scsi_task *task = NULL;
- task = iscsi_inquiry_sync(iscsi, lun, evpd, pc, 64);
+static struct scsi_task *iscsi_do_inquiry(struct iscsi_context *iscsi, int lun,
+ int evpd, int pc)
+{
+ int full_size;
+ struct scsi_task *task = NULL;
+ task = iscsi_inquiry_sync(iscsi, lun, evpd, pc, 64);
+ if (task == NULL || task->status != SCSI_STATUS_GOOD) {
+ goto fail;
+ }
+ full_size = scsi_datain_getfullsize(task);
+ if (full_size > task->datain.size) {
+ scsi_free_scsi_task(task);
+
+ /* we need more data for the full list */
+ task = iscsi_inquiry_sync(iscsi, lun, evpd, pc, full_size);
if (task == NULL || task->status != SCSI_STATUS_GOOD) {
goto fail;
}
- full_size = scsi_datain_getfullsize(task);
- if (full_size > task->datain.size) {
- scsi_free_scsi_task(task);
-
- /* we need more data for the full list */
- task = iscsi_inquiry_sync(iscsi, lun, evpd, pc, full_size);
- if (task == NULL || task->status != SCSI_STATUS_GOOD) {
- goto fail;
- }
- }
+ }
- return task;
+ return task;
fail:
- error_report("iSCSI: Inquiry command failed : %s",
- iscsi_get_error(iscsi));
- if (task) {
- scsi_free_scsi_task(task);
- return NULL;
- }
+ error_report("iSCSI: Inquiry command failed : %s",
+ iscsi_get_error(iscsi));
+ if (task) {
+ scsi_free_scsi_task(task);
return NULL;
+ }
+ return NULL;
}
/*
--
1.8.5.3
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [Qemu-devel] [PATCH 05/20] iscsi: fix indentation
2014-02-09 9:48 ` [Qemu-devel] [PATCH 05/20] iscsi: fix indentation Paolo Bonzini
@ 2014-02-10 7:48 ` Fam Zheng
0 siblings, 0 replies; 48+ messages in thread
From: Fam Zheng @ 2014-02-10 7:48 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, qemu-devel, stefanha, mreitz
On Sun, 02/09 10:48, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> block/iscsi.c | 45 +++++++++++++++++++++++----------------------
> 1 file changed, 23 insertions(+), 22 deletions(-)
>
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 6f4af72..e654a57 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -1065,35 +1065,36 @@ static QemuOptsList runtime_opts = {
> },
> };
>
> -static struct scsi_task *iscsi_do_inquiry(struct iscsi_context *iscsi,
> - int lun, int evpd, int pc) {
> - int full_size;
> - struct scsi_task *task = NULL;
> - task = iscsi_inquiry_sync(iscsi, lun, evpd, pc, 64);
> +static struct scsi_task *iscsi_do_inquiry(struct iscsi_context *iscsi, int lun,
> + int evpd, int pc)
> +{
> + int full_size;
> + struct scsi_task *task = NULL;
> + task = iscsi_inquiry_sync(iscsi, lun, evpd, pc, 64);
> + if (task == NULL || task->status != SCSI_STATUS_GOOD) {
> + goto fail;
> + }
> + full_size = scsi_datain_getfullsize(task);
> + if (full_size > task->datain.size) {
> + scsi_free_scsi_task(task);
> +
> + /* we need more data for the full list */
> + task = iscsi_inquiry_sync(iscsi, lun, evpd, pc, full_size);
> if (task == NULL || task->status != SCSI_STATUS_GOOD) {
> goto fail;
> }
> - full_size = scsi_datain_getfullsize(task);
> - if (full_size > task->datain.size) {
> - scsi_free_scsi_task(task);
> -
> - /* we need more data for the full list */
> - task = iscsi_inquiry_sync(iscsi, lun, evpd, pc, full_size);
> - if (task == NULL || task->status != SCSI_STATUS_GOOD) {
> - goto fail;
> - }
> - }
> + }
>
> - return task;
> + return task;
>
> fail:
> - error_report("iSCSI: Inquiry command failed : %s",
> - iscsi_get_error(iscsi));
> - if (task) {
> - scsi_free_scsi_task(task);
> - return NULL;
> - }
> + error_report("iSCSI: Inquiry command failed : %s",
> + iscsi_get_error(iscsi));
> + if (task) {
> + scsi_free_scsi_task(task);
> return NULL;
> + }
> + return NULL;
> }
>
> /*
> --
> 1.8.5.3
>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 48+ messages in thread
* [Qemu-devel] [PATCH 06/20] iscsi: correctly propagate errors in iscsi_open
2014-02-09 9:48 [Qemu-devel] [PATCH 00/20] Improve bdrv_open error messages Paolo Bonzini
` (4 preceding siblings ...)
2014-02-09 9:48 ` [Qemu-devel] [PATCH 05/20] iscsi: fix indentation Paolo Bonzini
@ 2014-02-09 9:48 ` Paolo Bonzini
2014-02-10 7:55 ` Fam Zheng
2014-02-09 9:48 ` [Qemu-devel] [PATCH 07/20] gluster: default scheme to gluster:// and host to localhost Paolo Bonzini
` (13 subsequent siblings)
19 siblings, 1 reply; 48+ messages in thread
From: Paolo Bonzini @ 2014-02-09 9:48 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, mreitz
Before:
$ ./qemu-io-old
qemu-io-old> open -r -o file.driver=iscsi,file.filename=foo
Failed to parse URL : foo
qemu-io-old: can't open device (null): Could not open 'foo': Invalid argument
After:
$ ./qemu-io
qemu-io> open -r -o file.driver=iscsi,file.filename=foo
qemu-io: can't open device (null): Failed to parse URL : foo
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/iscsi.c | 102 ++++++++++++++++++++++++++++++----------------------------
1 file changed, 52 insertions(+), 50 deletions(-)
diff --git a/block/iscsi.c b/block/iscsi.c
index e654a57..2cf1983 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -856,7 +856,8 @@ retry:
#endif /* SCSI_SENSE_ASCQ_CAPACITY_DATA_HAS_CHANGED */
-static int parse_chap(struct iscsi_context *iscsi, const char *target)
+static void parse_chap(struct iscsi_context *iscsi, const char *target,
+ Error **errp)
{
QemuOptsList *list;
QemuOpts *opts;
@@ -865,37 +866,35 @@ static int parse_chap(struct iscsi_context *iscsi, const char *target)
list = qemu_find_opts("iscsi");
if (!list) {
- return 0;
+ return;
}
opts = qemu_opts_find(list, target);
if (opts == NULL) {
opts = QTAILQ_FIRST(&list->head);
if (!opts) {
- return 0;
+ return;
}
}
user = qemu_opt_get(opts, "user");
if (!user) {
- return 0;
+ return;
}
password = qemu_opt_get(opts, "password");
if (!password) {
- error_report("CHAP username specified but no password was given");
- return -1;
+ error_setg(errp, "CHAP username specified but no password was given");
+ return;
}
if (iscsi_set_initiator_username_pwd(iscsi, user, password)) {
- error_report("Failed to set initiator username and password");
- return -1;
+ error_setg(errp, "Failed to set initiator username and password");
}
-
- return 0;
}
-static void parse_header_digest(struct iscsi_context *iscsi, const char *target)
+static void parse_header_digest(struct iscsi_context *iscsi, const char *target,
+ Error **errp)
{
QemuOptsList *list;
QemuOpts *opts;
@@ -928,7 +927,7 @@ static void parse_header_digest(struct iscsi_context *iscsi, const char *target)
} else if (!strcmp(digest, "NONE-CRC32C")) {
iscsi_set_header_digest(iscsi, ISCSI_HEADER_DIGEST_NONE_CRC32C);
} else {
- error_report("Invalid header-digest setting : %s", digest);
+ error_setg(errp, "Invalid header-digest setting : %s", digest);
}
}
@@ -986,12 +985,11 @@ static void iscsi_nop_timed_event(void *opaque)
}
#endif
-static int iscsi_readcapacity_sync(IscsiLun *iscsilun)
+static void iscsi_readcapacity_sync(IscsiLun *iscsilun, Error **errp)
{
struct scsi_task *task = NULL;
struct scsi_readcapacity10 *rc10 = NULL;
struct scsi_readcapacity16 *rc16 = NULL;
- int ret = 0;
int retries = ISCSI_CMD_RETRIES;
do {
@@ -1006,8 +1004,7 @@ static int iscsi_readcapacity_sync(IscsiLun *iscsilun)
if (task != NULL && task->status == SCSI_STATUS_GOOD) {
rc16 = scsi_datain_unmarshall(task);
if (rc16 == NULL) {
- error_report("iSCSI: Failed to unmarshall readcapacity16 data.");
- ret = -EINVAL;
+ error_setg(errp, "iSCSI: Failed to unmarshall readcapacity16 data.");
} else {
iscsilun->block_size = rc16->block_length;
iscsilun->num_blocks = rc16->returned_lba + 1;
@@ -1021,8 +1018,7 @@ static int iscsi_readcapacity_sync(IscsiLun *iscsilun)
if (task != NULL && task->status == SCSI_STATUS_GOOD) {
rc10 = scsi_datain_unmarshall(task);
if (rc10 == NULL) {
- error_report("iSCSI: Failed to unmarshall readcapacity10 data.");
- ret = -EINVAL;
+ error_setg(errp, "iSCSI: Failed to unmarshall readcapacity10 data.");
} else {
iscsilun->block_size = rc10->block_size;
if (rc10->lba == 0) {
@@ -1035,20 +1031,18 @@ static int iscsi_readcapacity_sync(IscsiLun *iscsilun)
}
break;
default:
- return 0;
+ return;
}
} while (task != NULL && task->status == SCSI_STATUS_CHECK_CONDITION
&& task->sense.key == SCSI_SENSE_UNIT_ATTENTION
&& retries-- > 0);
if (task == NULL || task->status != SCSI_STATUS_GOOD) {
- error_report("iSCSI: failed to send readcapacity10 command.");
- ret = -EINVAL;
+ error_setg(errp, "iSCSI: failed to send readcapacity10 command.");
}
if (task) {
scsi_free_scsi_task(task);
}
- return ret;
}
/* TODO Convert to fine grained options */
@@ -1066,7 +1060,7 @@ static QemuOptsList runtime_opts = {
};
static struct scsi_task *iscsi_do_inquiry(struct iscsi_context *iscsi, int lun,
- int evpd, int pc)
+ int evpd, int pc, Error **errp)
{
int full_size;
struct scsi_task *task = NULL;
@@ -1088,8 +1082,8 @@ static struct scsi_task *iscsi_do_inquiry(struct iscsi_context *iscsi, int lun,
return task;
fail:
- error_report("iSCSI: Inquiry command failed : %s",
- iscsi_get_error(iscsi));
+ error_setg(errp, "iSCSI: Inquiry command failed : %s",
+ iscsi_get_error(iscsi));
if (task) {
scsi_free_scsi_task(task);
return NULL;
@@ -1116,27 +1110,25 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
int ret;
if ((BDRV_SECTOR_SIZE % 512) != 0) {
- error_report("iSCSI: Invalid BDRV_SECTOR_SIZE. "
- "BDRV_SECTOR_SIZE(%lld) is not a multiple "
- "of 512", BDRV_SECTOR_SIZE);
+ error_setg(errp, "iSCSI: Invalid BDRV_SECTOR_SIZE. "
+ "BDRV_SECTOR_SIZE(%lld) is not a multiple "
+ "of 512", BDRV_SECTOR_SIZE);
return -EINVAL;
}
opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
qemu_opts_absorb_qdict(opts, options, &local_err);
if (error_is_set(&local_err)) {
- qerror_report_err(local_err);
- error_free(local_err);
+ error_propagate(errp, local_err);
ret = -EINVAL;
goto out;
}
filename = qemu_opt_get(opts, "filename");
-
iscsi_url = iscsi_parse_full_url(iscsi, filename);
if (iscsi_url == NULL) {
- error_report("Failed to parse URL : %s", filename);
+ error_setg(errp, "Failed to parse URL : %s", filename);
ret = -EINVAL;
goto out;
}
@@ -1147,13 +1139,13 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
iscsi = iscsi_create_context(initiator_name);
if (iscsi == NULL) {
- error_report("iSCSI: Failed to create iSCSI context.");
+ error_setg(errp, "iSCSI: Failed to create iSCSI context.");
ret = -ENOMEM;
goto out;
}
if (iscsi_set_targetname(iscsi, iscsi_url->target)) {
- error_report("iSCSI: Failed to set target name.");
+ error_setg(errp, "iSCSI: Failed to set target name.");
ret = -EINVAL;
goto out;
}
@@ -1162,21 +1154,22 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
ret = iscsi_set_initiator_username_pwd(iscsi, iscsi_url->user,
iscsi_url->passwd);
if (ret != 0) {
- error_report("Failed to set initiator username and password");
+ error_setg(errp, "Failed to set initiator username and password");
ret = -EINVAL;
goto out;
}
}
/* check if we got CHAP username/password via the options */
- if (parse_chap(iscsi, iscsi_url->target) != 0) {
- error_report("iSCSI: Failed to set CHAP user/password");
+ parse_chap(iscsi, iscsi_url->target, &local_err);
+ if (local_err != NULL) {
+ error_propagate(errp, local_err);
ret = -EINVAL;
goto out;
}
if (iscsi_set_session_type(iscsi, ISCSI_SESSION_NORMAL) != 0) {
- error_report("iSCSI: Failed to set session type to normal.");
+ error_setg(errp, "iSCSI: Failed to set session type to normal.");
ret = -EINVAL;
goto out;
}
@@ -1184,10 +1177,15 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
iscsi_set_header_digest(iscsi, ISCSI_HEADER_DIGEST_NONE_CRC32C);
/* check if we got HEADER_DIGEST via the options */
- parse_header_digest(iscsi, iscsi_url->target);
+ parse_header_digest(iscsi, iscsi_url->target, &local_err);
+ if (local_err != NULL) {
+ error_propagate(errp, local_err);
+ ret = -EINVAL;
+ goto out;
+ }
if (iscsi_full_connect_sync(iscsi, iscsi_url->portal, iscsi_url->lun) != 0) {
- error_report("iSCSI: Failed to connect to LUN : %s",
+ error_setg(errp, "iSCSI: Failed to connect to LUN : %s",
iscsi_get_error(iscsi));
ret = -EINVAL;
goto out;
@@ -1199,14 +1197,14 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
task = iscsi_inquiry_sync(iscsi, iscsilun->lun, 0, 0, 36);
if (task == NULL || task->status != SCSI_STATUS_GOOD) {
- error_report("iSCSI: failed to send inquiry command.");
+ error_setg(errp, "iSCSI: failed to send inquiry command.");
ret = -EINVAL;
goto out;
}
inq = scsi_datain_unmarshall(task);
if (inq == NULL) {
- error_report("iSCSI: Failed to unmarshall inquiry data.");
+ error_setg(errp, "iSCSI: Failed to unmarshall inquiry data.");
ret = -EINVAL;
goto out;
}
@@ -1214,7 +1212,9 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
iscsilun->type = inq->periperal_device_type;
iscsilun->has_write_same = true;
- if ((ret = iscsi_readcapacity_sync(iscsilun)) != 0) {
+ iscsi_readcapacity_sync(iscsilun, &local_err);
+ if (local_err != NULL) {
+ error_propagate(errp, local_err);
goto out;
}
bs->total_sectors = sector_lun2qemu(iscsilun->num_blocks, iscsilun);
@@ -1232,14 +1232,15 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
if (iscsilun->lbpme) {
struct scsi_inquiry_logical_block_provisioning *inq_lbp;
task = iscsi_do_inquiry(iscsilun->iscsi, iscsilun->lun, 1,
- SCSI_INQUIRY_PAGECODE_LOGICAL_BLOCK_PROVISIONING);
+ SCSI_INQUIRY_PAGECODE_LOGICAL_BLOCK_PROVISIONING,
+ errp);
if (task == NULL) {
ret = -EINVAL;
goto out;
}
inq_lbp = scsi_datain_unmarshall(task);
if (inq_lbp == NULL) {
- error_report("iSCSI: failed to unmarshall inquiry datain blob");
+ error_setg(errp, "iSCSI: failed to unmarshall inquiry datain blob");
ret = -EINVAL;
goto out;
}
@@ -1252,14 +1253,14 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
if (iscsilun->lbp.lbpu || iscsilun->lbp.lbpws) {
struct scsi_inquiry_block_limits *inq_bl;
task = iscsi_do_inquiry(iscsilun->iscsi, iscsilun->lun, 1,
- SCSI_INQUIRY_PAGECODE_BLOCK_LIMITS);
+ SCSI_INQUIRY_PAGECODE_BLOCK_LIMITS, errp);
if (task == NULL) {
ret = -EINVAL;
goto out;
}
inq_bl = scsi_datain_unmarshall(task);
if (inq_bl == NULL) {
- error_report("iSCSI: failed to unmarshall inquiry datain blob");
+ error_setg(errp, "iSCSI: failed to unmarshall inquiry datain blob");
ret = -EINVAL;
goto out;
}
@@ -1349,14 +1350,15 @@ static int iscsi_reopen_prepare(BDRVReopenState *state,
static int iscsi_truncate(BlockDriverState *bs, int64_t offset)
{
IscsiLun *iscsilun = bs->opaque;
- int ret = 0;
+ Error *local_err = NULL;
if (iscsilun->type != TYPE_DISK) {
return -ENOTSUP;
}
- if ((ret = iscsi_readcapacity_sync(iscsilun)) != 0) {
- return ret;
+ iscsi_readcapacity_sync(iscsilun, &local_err);
+ if (local_err != NULL) {
+ return -EIO;
}
if (offset > iscsi_getlength(bs)) {
--
1.8.5.3
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [Qemu-devel] [PATCH 06/20] iscsi: correctly propagate errors in iscsi_open
2014-02-09 9:48 ` [Qemu-devel] [PATCH 06/20] iscsi: correctly propagate errors in iscsi_open Paolo Bonzini
@ 2014-02-10 7:55 ` Fam Zheng
0 siblings, 0 replies; 48+ messages in thread
From: Fam Zheng @ 2014-02-10 7:55 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, qemu-devel, stefanha, mreitz
On Sun, 02/09 10:48, Paolo Bonzini wrote:
> Before:
> $ ./qemu-io-old
> qemu-io-old> open -r -o file.driver=iscsi,file.filename=foo
> Failed to parse URL : foo
> qemu-io-old: can't open device (null): Could not open 'foo': Invalid argument
>
> After:
> $ ./qemu-io
> qemu-io> open -r -o file.driver=iscsi,file.filename=foo
> qemu-io: can't open device (null): Failed to parse URL : foo
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> block/iscsi.c | 102 ++++++++++++++++++++++++++++++----------------------------
> 1 file changed, 52 insertions(+), 50 deletions(-)
>
> diff --git a/block/iscsi.c b/block/iscsi.c
> index e654a57..2cf1983 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -856,7 +856,8 @@ retry:
>
> #endif /* SCSI_SENSE_ASCQ_CAPACITY_DATA_HAS_CHANGED */
>
> -static int parse_chap(struct iscsi_context *iscsi, const char *target)
> +static void parse_chap(struct iscsi_context *iscsi, const char *target,
> + Error **errp)
> {
> QemuOptsList *list;
> QemuOpts *opts;
> @@ -865,37 +866,35 @@ static int parse_chap(struct iscsi_context *iscsi, const char *target)
>
> list = qemu_find_opts("iscsi");
> if (!list) {
> - return 0;
> + return;
> }
>
> opts = qemu_opts_find(list, target);
> if (opts == NULL) {
> opts = QTAILQ_FIRST(&list->head);
> if (!opts) {
> - return 0;
> + return;
> }
> }
>
> user = qemu_opt_get(opts, "user");
> if (!user) {
> - return 0;
> + return;
> }
>
> password = qemu_opt_get(opts, "password");
> if (!password) {
> - error_report("CHAP username specified but no password was given");
> - return -1;
> + error_setg(errp, "CHAP username specified but no password was given");
> + return;
> }
>
> if (iscsi_set_initiator_username_pwd(iscsi, user, password)) {
> - error_report("Failed to set initiator username and password");
> - return -1;
> + error_setg(errp, "Failed to set initiator username and password");
> }
> -
> - return 0;
> }
>
> -static void parse_header_digest(struct iscsi_context *iscsi, const char *target)
> +static void parse_header_digest(struct iscsi_context *iscsi, const char *target,
> + Error **errp)
> {
> QemuOptsList *list;
> QemuOpts *opts;
> @@ -928,7 +927,7 @@ static void parse_header_digest(struct iscsi_context *iscsi, const char *target)
> } else if (!strcmp(digest, "NONE-CRC32C")) {
> iscsi_set_header_digest(iscsi, ISCSI_HEADER_DIGEST_NONE_CRC32C);
> } else {
> - error_report("Invalid header-digest setting : %s", digest);
> + error_setg(errp, "Invalid header-digest setting : %s", digest);
> }
> }
>
> @@ -986,12 +985,11 @@ static void iscsi_nop_timed_event(void *opaque)
> }
> #endif
>
> -static int iscsi_readcapacity_sync(IscsiLun *iscsilun)
> +static void iscsi_readcapacity_sync(IscsiLun *iscsilun, Error **errp)
> {
> struct scsi_task *task = NULL;
> struct scsi_readcapacity10 *rc10 = NULL;
> struct scsi_readcapacity16 *rc16 = NULL;
> - int ret = 0;
> int retries = ISCSI_CMD_RETRIES;
>
> do {
> @@ -1006,8 +1004,7 @@ static int iscsi_readcapacity_sync(IscsiLun *iscsilun)
> if (task != NULL && task->status == SCSI_STATUS_GOOD) {
> rc16 = scsi_datain_unmarshall(task);
> if (rc16 == NULL) {
> - error_report("iSCSI: Failed to unmarshall readcapacity16 data.");
> - ret = -EINVAL;
> + error_setg(errp, "iSCSI: Failed to unmarshall readcapacity16 data.");
> } else {
> iscsilun->block_size = rc16->block_length;
> iscsilun->num_blocks = rc16->returned_lba + 1;
> @@ -1021,8 +1018,7 @@ static int iscsi_readcapacity_sync(IscsiLun *iscsilun)
> if (task != NULL && task->status == SCSI_STATUS_GOOD) {
> rc10 = scsi_datain_unmarshall(task);
> if (rc10 == NULL) {
> - error_report("iSCSI: Failed to unmarshall readcapacity10 data.");
> - ret = -EINVAL;
> + error_setg(errp, "iSCSI: Failed to unmarshall readcapacity10 data.");
> } else {
> iscsilun->block_size = rc10->block_size;
> if (rc10->lba == 0) {
> @@ -1035,20 +1031,18 @@ static int iscsi_readcapacity_sync(IscsiLun *iscsilun)
> }
> break;
> default:
> - return 0;
> + return;
> }
> } while (task != NULL && task->status == SCSI_STATUS_CHECK_CONDITION
> && task->sense.key == SCSI_SENSE_UNIT_ATTENTION
> && retries-- > 0);
>
> if (task == NULL || task->status != SCSI_STATUS_GOOD) {
> - error_report("iSCSI: failed to send readcapacity10 command.");
> - ret = -EINVAL;
> + error_setg(errp, "iSCSI: failed to send readcapacity10 command.");
> }
> if (task) {
> scsi_free_scsi_task(task);
> }
> - return ret;
> }
>
> /* TODO Convert to fine grained options */
> @@ -1066,7 +1060,7 @@ static QemuOptsList runtime_opts = {
> };
>
> static struct scsi_task *iscsi_do_inquiry(struct iscsi_context *iscsi, int lun,
> - int evpd, int pc)
> + int evpd, int pc, Error **errp)
> {
> int full_size;
> struct scsi_task *task = NULL;
> @@ -1088,8 +1082,8 @@ static struct scsi_task *iscsi_do_inquiry(struct iscsi_context *iscsi, int lun,
> return task;
>
> fail:
> - error_report("iSCSI: Inquiry command failed : %s",
> - iscsi_get_error(iscsi));
> + error_setg(errp, "iSCSI: Inquiry command failed : %s",
> + iscsi_get_error(iscsi));
> if (task) {
> scsi_free_scsi_task(task);
> return NULL;
> @@ -1116,27 +1110,25 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
> int ret;
>
> if ((BDRV_SECTOR_SIZE % 512) != 0) {
> - error_report("iSCSI: Invalid BDRV_SECTOR_SIZE. "
> - "BDRV_SECTOR_SIZE(%lld) is not a multiple "
> - "of 512", BDRV_SECTOR_SIZE);
> + error_setg(errp, "iSCSI: Invalid BDRV_SECTOR_SIZE. "
> + "BDRV_SECTOR_SIZE(%lld) is not a multiple "
> + "of 512", BDRV_SECTOR_SIZE);
> return -EINVAL;
> }
>
> opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
> qemu_opts_absorb_qdict(opts, options, &local_err);
> if (error_is_set(&local_err)) {
> - qerror_report_err(local_err);
> - error_free(local_err);
> + error_propagate(errp, local_err);
> ret = -EINVAL;
> goto out;
> }
>
> filename = qemu_opt_get(opts, "filename");
>
> -
> iscsi_url = iscsi_parse_full_url(iscsi, filename);
> if (iscsi_url == NULL) {
> - error_report("Failed to parse URL : %s", filename);
> + error_setg(errp, "Failed to parse URL : %s", filename);
> ret = -EINVAL;
> goto out;
> }
> @@ -1147,13 +1139,13 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
>
> iscsi = iscsi_create_context(initiator_name);
> if (iscsi == NULL) {
> - error_report("iSCSI: Failed to create iSCSI context.");
> + error_setg(errp, "iSCSI: Failed to create iSCSI context.");
> ret = -ENOMEM;
> goto out;
> }
>
> if (iscsi_set_targetname(iscsi, iscsi_url->target)) {
> - error_report("iSCSI: Failed to set target name.");
> + error_setg(errp, "iSCSI: Failed to set target name.");
> ret = -EINVAL;
> goto out;
> }
> @@ -1162,21 +1154,22 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
> ret = iscsi_set_initiator_username_pwd(iscsi, iscsi_url->user,
> iscsi_url->passwd);
> if (ret != 0) {
> - error_report("Failed to set initiator username and password");
> + error_setg(errp, "Failed to set initiator username and password");
> ret = -EINVAL;
> goto out;
> }
> }
>
> /* check if we got CHAP username/password via the options */
> - if (parse_chap(iscsi, iscsi_url->target) != 0) {
> - error_report("iSCSI: Failed to set CHAP user/password");
> + parse_chap(iscsi, iscsi_url->target, &local_err);
> + if (local_err != NULL) {
Should we use error_is_set()?
> + error_propagate(errp, local_err);
> ret = -EINVAL;
> goto out;
> }
>
> if (iscsi_set_session_type(iscsi, ISCSI_SESSION_NORMAL) != 0) {
> - error_report("iSCSI: Failed to set session type to normal.");
> + error_setg(errp, "iSCSI: Failed to set session type to normal.");
> ret = -EINVAL;
> goto out;
> }
> @@ -1184,10 +1177,15 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
> iscsi_set_header_digest(iscsi, ISCSI_HEADER_DIGEST_NONE_CRC32C);
>
> /* check if we got HEADER_DIGEST via the options */
> - parse_header_digest(iscsi, iscsi_url->target);
> + parse_header_digest(iscsi, iscsi_url->target, &local_err);
> + if (local_err != NULL) {
Same here.
> + error_propagate(errp, local_err);
> + ret = -EINVAL;
> + goto out;
> + }
>
> if (iscsi_full_connect_sync(iscsi, iscsi_url->portal, iscsi_url->lun) != 0) {
> - error_report("iSCSI: Failed to connect to LUN : %s",
> + error_setg(errp, "iSCSI: Failed to connect to LUN : %s",
> iscsi_get_error(iscsi));
> ret = -EINVAL;
> goto out;
> @@ -1199,14 +1197,14 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
> task = iscsi_inquiry_sync(iscsi, iscsilun->lun, 0, 0, 36);
>
> if (task == NULL || task->status != SCSI_STATUS_GOOD) {
> - error_report("iSCSI: failed to send inquiry command.");
> + error_setg(errp, "iSCSI: failed to send inquiry command.");
> ret = -EINVAL;
> goto out;
> }
>
> inq = scsi_datain_unmarshall(task);
> if (inq == NULL) {
> - error_report("iSCSI: Failed to unmarshall inquiry data.");
> + error_setg(errp, "iSCSI: Failed to unmarshall inquiry data.");
> ret = -EINVAL;
> goto out;
> }
> @@ -1214,7 +1212,9 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
> iscsilun->type = inq->periperal_device_type;
> iscsilun->has_write_same = true;
>
> - if ((ret = iscsi_readcapacity_sync(iscsilun)) != 0) {
> + iscsi_readcapacity_sync(iscsilun, &local_err);
> + if (local_err != NULL) {
And here.
> + error_propagate(errp, local_err);
> goto out;
> }
> bs->total_sectors = sector_lun2qemu(iscsilun->num_blocks, iscsilun);
> @@ -1232,14 +1232,15 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
> if (iscsilun->lbpme) {
> struct scsi_inquiry_logical_block_provisioning *inq_lbp;
> task = iscsi_do_inquiry(iscsilun->iscsi, iscsilun->lun, 1,
> - SCSI_INQUIRY_PAGECODE_LOGICAL_BLOCK_PROVISIONING);
> + SCSI_INQUIRY_PAGECODE_LOGICAL_BLOCK_PROVISIONING,
> + errp);
> if (task == NULL) {
> ret = -EINVAL;
> goto out;
> }
> inq_lbp = scsi_datain_unmarshall(task);
> if (inq_lbp == NULL) {
> - error_report("iSCSI: failed to unmarshall inquiry datain blob");
> + error_setg(errp, "iSCSI: failed to unmarshall inquiry datain blob");
> ret = -EINVAL;
> goto out;
> }
> @@ -1252,14 +1253,14 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
> if (iscsilun->lbp.lbpu || iscsilun->lbp.lbpws) {
> struct scsi_inquiry_block_limits *inq_bl;
> task = iscsi_do_inquiry(iscsilun->iscsi, iscsilun->lun, 1,
> - SCSI_INQUIRY_PAGECODE_BLOCK_LIMITS);
> + SCSI_INQUIRY_PAGECODE_BLOCK_LIMITS, errp);
> if (task == NULL) {
> ret = -EINVAL;
> goto out;
> }
> inq_bl = scsi_datain_unmarshall(task);
> if (inq_bl == NULL) {
> - error_report("iSCSI: failed to unmarshall inquiry datain blob");
> + error_setg(errp, "iSCSI: failed to unmarshall inquiry datain blob");
> ret = -EINVAL;
> goto out;
> }
> @@ -1349,14 +1350,15 @@ static int iscsi_reopen_prepare(BDRVReopenState *state,
> static int iscsi_truncate(BlockDriverState *bs, int64_t offset)
> {
> IscsiLun *iscsilun = bs->opaque;
> - int ret = 0;
> + Error *local_err = NULL;
>
> if (iscsilun->type != TYPE_DISK) {
> return -ENOTSUP;
> }
>
> - if ((ret = iscsi_readcapacity_sync(iscsilun)) != 0) {
> - return ret;
> + iscsi_readcapacity_sync(iscsilun, &local_err);
> + if (local_err != NULL) {
And here.
And local_err needs error_free() before returning.
Thanks,
Fam
> + return -EIO;
> }
>
> if (offset > iscsi_getlength(bs)) {
> --
> 1.8.5.3
>
>
>
^ permalink raw reply [flat|nested] 48+ messages in thread
* [Qemu-devel] [PATCH 07/20] gluster: default scheme to gluster:// and host to localhost.
2014-02-09 9:48 [Qemu-devel] [PATCH 00/20] Improve bdrv_open error messages Paolo Bonzini
` (5 preceding siblings ...)
2014-02-09 9:48 ` [Qemu-devel] [PATCH 06/20] iscsi: correctly propagate errors in iscsi_open Paolo Bonzini
@ 2014-02-09 9:48 ` Paolo Bonzini
2014-02-10 7:57 ` Fam Zheng
2014-02-09 9:48 ` [Qemu-devel] [PATCH 08/20] gluster: correctly propagate errors Paolo Bonzini
` (12 subsequent siblings)
19 siblings, 1 reply; 48+ messages in thread
From: Paolo Bonzini @ 2014-02-09 9:48 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, mreitz
Currently, "gluster:///volname/img" and (using file. options)
"file.driver=gluster,file.filename=foo" will segfault. Also,
"//host/volname/img" will be rejected, but it is a valid URL
that should be accepted just fine with "file.driver=gluster".
Accept all of these, by inferring missing transport and host
as TCP and localhost respectively.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/gluster.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/block/gluster.c b/block/gluster.c
index a009b15..f9dd37f 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -127,7 +127,7 @@ static int qemu_gluster_parseuri(GlusterConf *gconf, const char *filename)
}
/* transport */
- if (!strcmp(uri->scheme, "gluster")) {
+ if (!uri->scheme || !strcmp(uri->scheme, "gluster")) {
gconf->transport = g_strdup("tcp");
} else if (!strcmp(uri->scheme, "gluster+tcp")) {
gconf->transport = g_strdup("tcp");
@@ -163,7 +163,7 @@ static int qemu_gluster_parseuri(GlusterConf *gconf, const char *filename)
}
gconf->server = g_strdup(qp->p[0].value);
} else {
- gconf->server = g_strdup(uri->server);
+ gconf->server = g_strdup(uri->server ? uri->server : "localhost");
gconf->port = uri->port;
}
--
1.8.5.3
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [Qemu-devel] [PATCH 07/20] gluster: default scheme to gluster:// and host to localhost.
2014-02-09 9:48 ` [Qemu-devel] [PATCH 07/20] gluster: default scheme to gluster:// and host to localhost Paolo Bonzini
@ 2014-02-10 7:57 ` Fam Zheng
0 siblings, 0 replies; 48+ messages in thread
From: Fam Zheng @ 2014-02-10 7:57 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, qemu-devel, stefanha, mreitz
On Sun, 02/09 10:48, Paolo Bonzini wrote:
> Currently, "gluster:///volname/img" and (using file. options)
> "file.driver=gluster,file.filename=foo" will segfault. Also,
> "//host/volname/img" will be rejected, but it is a valid URL
> that should be accepted just fine with "file.driver=gluster".
> Accept all of these, by inferring missing transport and host
> as TCP and localhost respectively.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> block/gluster.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/block/gluster.c b/block/gluster.c
> index a009b15..f9dd37f 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -127,7 +127,7 @@ static int qemu_gluster_parseuri(GlusterConf *gconf, const char *filename)
> }
>
> /* transport */
> - if (!strcmp(uri->scheme, "gluster")) {
> + if (!uri->scheme || !strcmp(uri->scheme, "gluster")) {
> gconf->transport = g_strdup("tcp");
> } else if (!strcmp(uri->scheme, "gluster+tcp")) {
> gconf->transport = g_strdup("tcp");
> @@ -163,7 +163,7 @@ static int qemu_gluster_parseuri(GlusterConf *gconf, const char *filename)
> }
> gconf->server = g_strdup(qp->p[0].value);
> } else {
> - gconf->server = g_strdup(uri->server);
> + gconf->server = g_strdup(uri->server ? uri->server : "localhost");
> gconf->port = uri->port;
> }
>
> --
> 1.8.5.3
>
>
>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 48+ messages in thread
* [Qemu-devel] [PATCH 08/20] gluster: correctly propagate errors
2014-02-09 9:48 [Qemu-devel] [PATCH 00/20] Improve bdrv_open error messages Paolo Bonzini
` (6 preceding siblings ...)
2014-02-09 9:48 ` [Qemu-devel] [PATCH 07/20] gluster: default scheme to gluster:// and host to localhost Paolo Bonzini
@ 2014-02-09 9:48 ` Paolo Bonzini
2014-02-10 8:02 ` Fam Zheng
2014-02-09 9:48 ` [Qemu-devel] [PATCH 09/20] cow: " Paolo Bonzini
` (11 subsequent siblings)
19 siblings, 1 reply; 48+ messages in thread
From: Paolo Bonzini @ 2014-02-09 9:48 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, mreitz
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/gluster.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/block/gluster.c b/block/gluster.c
index f9dd37f..bc9c59f 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -175,7 +175,8 @@ out:
return ret;
}
-static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename)
+static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename,
+ Error **errp)
{
struct glfs *glfs = NULL;
int ret;
@@ -183,8 +184,8 @@ static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename)
ret = qemu_gluster_parseuri(gconf, filename);
if (ret < 0) {
- error_report("Usage: file=gluster[+transport]://[server[:port]]/"
- "volname/image[?socket=...]");
+ error_setg(errp, "Usage: file=gluster[+transport]://[server[:port]]/"
+ "volname/image[?socket=...]");
errno = -ret;
goto out;
}
@@ -211,9 +212,11 @@ static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename)
ret = glfs_init(glfs);
if (ret) {
- error_report("Gluster connection failed for server=%s port=%d "
- "volume=%s image=%s transport=%s", gconf->server, gconf->port,
- gconf->volname, gconf->image, gconf->transport);
+ error_setg_errno(errp, errno,
+ "Gluster connection failed for server=%s port=%d "
+ "volume=%s image=%s transport=%s", gconf->server,
+ gconf->port, gconf->volname, gconf->image,
+ gconf->transport);
goto out;
}
return glfs;
@@ -283,15 +286,14 @@ static int qemu_gluster_open(BlockDriverState *bs, QDict *options,
opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
qemu_opts_absorb_qdict(opts, options, &local_err);
if (error_is_set(&local_err)) {
- qerror_report_err(local_err);
- error_free(local_err);
+ error_propagate(errp, local_err);
ret = -EINVAL;
goto out;
}
filename = qemu_opt_get(opts, "filename");
- s->glfs = qemu_gluster_init(gconf, filename);
+ s->glfs = qemu_gluster_init(gconf, filename, errp);
if (!s->glfs) {
ret = -errno;
goto out;
@@ -389,9 +391,9 @@ static int qemu_gluster_create(const char *filename,
int64_t total_size = 0;
GlusterConf *gconf = g_malloc0(sizeof(GlusterConf));
- glfs = qemu_gluster_init(gconf, filename);
+ glfs = qemu_gluster_init(gconf, filename, errp);
if (!glfs) {
- ret = -errno;
+ ret = -EINVAL;
goto out;
}
--
1.8.5.3
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [Qemu-devel] [PATCH 08/20] gluster: correctly propagate errors
2014-02-09 9:48 ` [Qemu-devel] [PATCH 08/20] gluster: correctly propagate errors Paolo Bonzini
@ 2014-02-10 8:02 ` Fam Zheng
2014-02-10 8:27 ` Paolo Bonzini
0 siblings, 1 reply; 48+ messages in thread
From: Fam Zheng @ 2014-02-10 8:02 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, qemu-devel, stefanha, mreitz
On Sun, 02/09 10:48, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> block/gluster.c | 24 +++++++++++++-----------
> 1 file changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/block/gluster.c b/block/gluster.c
> index f9dd37f..bc9c59f 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -175,7 +175,8 @@ out:
> return ret;
> }
>
> -static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename)
> +static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename,
> + Error **errp)
> {
> struct glfs *glfs = NULL;
> int ret;
> @@ -183,8 +184,8 @@ static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename)
>
> ret = qemu_gluster_parseuri(gconf, filename);
> if (ret < 0) {
> - error_report("Usage: file=gluster[+transport]://[server[:port]]/"
> - "volname/image[?socket=...]");
> + error_setg(errp, "Usage: file=gluster[+transport]://[server[:port]]/"
> + "volname/image[?socket=...]");
> errno = -ret;
> goto out;
> }
> @@ -211,9 +212,11 @@ static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename)
>
> ret = glfs_init(glfs);
> if (ret) {
> - error_report("Gluster connection failed for server=%s port=%d "
> - "volume=%s image=%s transport=%s", gconf->server, gconf->port,
> - gconf->volname, gconf->image, gconf->transport);
> + error_setg_errno(errp, errno,
> + "Gluster connection failed for server=%s port=%d "
> + "volume=%s image=%s transport=%s", gconf->server,
> + gconf->port, gconf->volname, gconf->image,
> + gconf->transport);
> goto out;
> }
> return glfs;
> @@ -283,15 +286,14 @@ static int qemu_gluster_open(BlockDriverState *bs, QDict *options,
> opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
> qemu_opts_absorb_qdict(opts, options, &local_err);
> if (error_is_set(&local_err)) {
> - qerror_report_err(local_err);
> - error_free(local_err);
> + error_propagate(errp, local_err);
> ret = -EINVAL;
> goto out;
> }
>
> filename = qemu_opt_get(opts, "filename");
>
> - s->glfs = qemu_gluster_init(gconf, filename);
> + s->glfs = qemu_gluster_init(gconf, filename, errp);
> if (!s->glfs) {
> ret = -errno;
> goto out;
> @@ -389,9 +391,9 @@ static int qemu_gluster_create(const char *filename,
> int64_t total_size = 0;
> GlusterConf *gconf = g_malloc0(sizeof(GlusterConf));
>
> - glfs = qemu_gluster_init(gconf, filename);
> + glfs = qemu_gluster_init(gconf, filename, errp);
> if (!glfs) {
> - ret = -errno;
> + ret = -EINVAL;
Why dropping -errno here?
Thanks,
Fam
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [Qemu-devel] [PATCH 08/20] gluster: correctly propagate errors
2014-02-10 8:02 ` Fam Zheng
@ 2014-02-10 8:27 ` Paolo Bonzini
2014-02-10 9:13 ` Fam Zheng
0 siblings, 1 reply; 48+ messages in thread
From: Paolo Bonzini @ 2014-02-10 8:27 UTC (permalink / raw)
To: Fam Zheng; +Cc: kwolf, qemu-devel, stefanha, mreitz
Il 10/02/2014 09:02, Fam Zheng ha scritto:
> On Sun, 02/09 10:48, Paolo Bonzini wrote:
>> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> > ---
>> > block/gluster.c | 24 +++++++++++++-----------
>> > 1 file changed, 13 insertions(+), 11 deletions(-)
>> >
>> > diff --git a/block/gluster.c b/block/gluster.c
>> > index f9dd37f..bc9c59f 100644
>> > --- a/block/gluster.c
>> > +++ b/block/gluster.c
>> > @@ -175,7 +175,8 @@ out:
>> > return ret;
>> > }
>> >
>> > -static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename)
>> > +static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename,
>> > + Error **errp)
>> > {
>> > struct glfs *glfs = NULL;
>> > int ret;
>> > @@ -183,8 +184,8 @@ static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename)
>> >
>> > ret = qemu_gluster_parseuri(gconf, filename);
>> > if (ret < 0) {
>> > - error_report("Usage: file=gluster[+transport]://[server[:port]]/"
>> > - "volname/image[?socket=...]");
>> > + error_setg(errp, "Usage: file=gluster[+transport]://[server[:port]]/"
>> > + "volname/image[?socket=...]");
>> > errno = -ret;
>> > goto out;
>> > }
>> > @@ -211,9 +212,11 @@ static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename)
>> >
>> > ret = glfs_init(glfs);
>> > if (ret) {
>> > - error_report("Gluster connection failed for server=%s port=%d "
>> > - "volume=%s image=%s transport=%s", gconf->server, gconf->port,
>> > - gconf->volname, gconf->image, gconf->transport);
>> > + error_setg_errno(errp, errno,
>> > + "Gluster connection failed for server=%s port=%d "
>> > + "volume=%s image=%s transport=%s", gconf->server,
>> > + gconf->port, gconf->volname, gconf->image,
>> > + gconf->transport);
>> > goto out;
>> > }
>> > return glfs;
>> > @@ -283,15 +286,14 @@ static int qemu_gluster_open(BlockDriverState *bs, QDict *options,
>> > opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
>> > qemu_opts_absorb_qdict(opts, options, &local_err);
>> > if (error_is_set(&local_err)) {
>> > - qerror_report_err(local_err);
>> > - error_free(local_err);
>> > + error_propagate(errp, local_err);
>> > ret = -EINVAL;
>> > goto out;
>> > }
>> >
>> > filename = qemu_opt_get(opts, "filename");
>> >
>> > - s->glfs = qemu_gluster_init(gconf, filename);
>> > + s->glfs = qemu_gluster_init(gconf, filename, errp);
>> > if (!s->glfs) {
>> > ret = -errno;
>> > goto out;
>> > @@ -389,9 +391,9 @@ static int qemu_gluster_create(const char *filename,
>> > int64_t total_size = 0;
>> > GlusterConf *gconf = g_malloc0(sizeof(GlusterConf));
>> >
>> > - glfs = qemu_gluster_init(gconf, filename);
>> > + glfs = qemu_gluster_init(gconf, filename, errp);
>> > if (!glfs) {
>> > - ret = -errno;
>> > + ret = -EINVAL;
> Why dropping -errno here?
Because it is not applicable for the usage error messages. Anyway
nothing is lost because the errno is already embedded in the error if
applicable. If the Error* is set to non-NULL, the caller of
drv->bdrv_open ignores the return value except to check that it is <0.
Paolo
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [Qemu-devel] [PATCH 08/20] gluster: correctly propagate errors
2014-02-10 8:27 ` Paolo Bonzini
@ 2014-02-10 9:13 ` Fam Zheng
0 siblings, 0 replies; 48+ messages in thread
From: Fam Zheng @ 2014-02-10 9:13 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, qemu-devel, stefanha, mreitz
On Mon, 02/10 09:27, Paolo Bonzini wrote:
> Il 10/02/2014 09:02, Fam Zheng ha scritto:
> >On Sun, 02/09 10:48, Paolo Bonzini wrote:
> >>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >>> ---
> >>> block/gluster.c | 24 +++++++++++++-----------
> >>> 1 file changed, 13 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/block/gluster.c b/block/gluster.c
> >>> index f9dd37f..bc9c59f 100644
> >>> --- a/block/gluster.c
> >>> +++ b/block/gluster.c
> >>> @@ -175,7 +175,8 @@ out:
> >>> return ret;
> >>> }
> >>>
> >>> -static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename)
> >>> +static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename,
> >>> + Error **errp)
> >>> {
> >>> struct glfs *glfs = NULL;
> >>> int ret;
> >>> @@ -183,8 +184,8 @@ static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename)
> >>>
> >>> ret = qemu_gluster_parseuri(gconf, filename);
> >>> if (ret < 0) {
> >>> - error_report("Usage: file=gluster[+transport]://[server[:port]]/"
> >>> - "volname/image[?socket=...]");
> >>> + error_setg(errp, "Usage: file=gluster[+transport]://[server[:port]]/"
> >>> + "volname/image[?socket=...]");
> >>> errno = -ret;
> >>> goto out;
> >>> }
> >>> @@ -211,9 +212,11 @@ static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename)
> >>>
> >>> ret = glfs_init(glfs);
> >>> if (ret) {
> >>> - error_report("Gluster connection failed for server=%s port=%d "
> >>> - "volume=%s image=%s transport=%s", gconf->server, gconf->port,
> >>> - gconf->volname, gconf->image, gconf->transport);
> >>> + error_setg_errno(errp, errno,
> >>> + "Gluster connection failed for server=%s port=%d "
> >>> + "volume=%s image=%s transport=%s", gconf->server,
> >>> + gconf->port, gconf->volname, gconf->image,
> >>> + gconf->transport);
> >>> goto out;
> >>> }
> >>> return glfs;
> >>> @@ -283,15 +286,14 @@ static int qemu_gluster_open(BlockDriverState *bs, QDict *options,
> >>> opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
> >>> qemu_opts_absorb_qdict(opts, options, &local_err);
> >>> if (error_is_set(&local_err)) {
> >>> - qerror_report_err(local_err);
> >>> - error_free(local_err);
> >>> + error_propagate(errp, local_err);
> >>> ret = -EINVAL;
> >>> goto out;
> >>> }
> >>>
> >>> filename = qemu_opt_get(opts, "filename");
> >>>
> >>> - s->glfs = qemu_gluster_init(gconf, filename);
> >>> + s->glfs = qemu_gluster_init(gconf, filename, errp);
> >>> if (!s->glfs) {
> >>> ret = -errno;
> >>> goto out;
> >>> @@ -389,9 +391,9 @@ static int qemu_gluster_create(const char *filename,
> >>> int64_t total_size = 0;
> >>> GlusterConf *gconf = g_malloc0(sizeof(GlusterConf));
> >>>
> >>> - glfs = qemu_gluster_init(gconf, filename);
> >>> + glfs = qemu_gluster_init(gconf, filename, errp);
> >>> if (!glfs) {
> >>> - ret = -errno;
> >>> + ret = -EINVAL;
> >Why dropping -errno here?
>
> Because it is not applicable for the usage error messages. Anyway nothing
> is lost because the errno is already embedded in the error if applicable.
> If the Error* is set to non-NULL, the caller of drv->bdrv_open ignores the
> return value except to check that it is <0.
>
OK, then,
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 48+ messages in thread
* [Qemu-devel] [PATCH 09/20] cow: correctly propagate errors
2014-02-09 9:48 [Qemu-devel] [PATCH 00/20] Improve bdrv_open error messages Paolo Bonzini
` (7 preceding siblings ...)
2014-02-09 9:48 ` [Qemu-devel] [PATCH 08/20] gluster: correctly propagate errors Paolo Bonzini
@ 2014-02-09 9:48 ` Paolo Bonzini
2014-02-10 8:04 ` Fam Zheng
2014-02-09 9:48 ` [Qemu-devel] [PATCH 10/20] curl: " Paolo Bonzini
` (10 subsequent siblings)
19 siblings, 1 reply; 48+ messages in thread
From: Paolo Bonzini @ 2014-02-09 9:48 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, mreitz
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/cow.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/block/cow.c b/block/cow.c
index 7fc0b12..43a2150 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -82,7 +82,7 @@ static int cow_open(BlockDriverState *bs, QDict *options, int flags,
char version[64];
snprintf(version, sizeof(version),
"COW version %d", cow_header.version);
- qerror_report(QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
+ error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
bs->device_name, "cow", version);
ret = -ENOTSUP;
goto fail;
@@ -330,7 +330,6 @@ static int cow_create(const char *filename, QEMUOptionParameter *options,
struct stat st;
int64_t image_sectors = 0;
const char *image_filename = NULL;
- Error *local_err = NULL;
int ret;
BlockDriverState *cow_bs;
@@ -344,18 +343,13 @@ static int cow_create(const char *filename, QEMUOptionParameter *options,
options++;
}
- ret = bdrv_create_file(filename, options, &local_err);
+ ret = bdrv_create_file(filename, options, errp);
if (ret < 0) {
- qerror_report_err(local_err);
- error_free(local_err);
return ret;
}
- ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR,
- &local_err);
+ ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR, errp);
if (ret < 0) {
- qerror_report_err(local_err);
- error_free(local_err);
return ret;
}
--
1.8.5.3
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [Qemu-devel] [PATCH 09/20] cow: correctly propagate errors
2014-02-09 9:48 ` [Qemu-devel] [PATCH 09/20] cow: " Paolo Bonzini
@ 2014-02-10 8:04 ` Fam Zheng
2014-02-10 8:28 ` Paolo Bonzini
0 siblings, 1 reply; 48+ messages in thread
From: Fam Zheng @ 2014-02-10 8:04 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, qemu-devel, stefanha, mreitz
On Sun, 02/09 10:48, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> block/cow.c | 12 +++---------
> 1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/block/cow.c b/block/cow.c
> index 7fc0b12..43a2150 100644
> --- a/block/cow.c
> +++ b/block/cow.c
> @@ -82,7 +82,7 @@ static int cow_open(BlockDriverState *bs, QDict *options, int flags,
> char version[64];
> snprintf(version, sizeof(version),
> "COW version %d", cow_header.version);
> - qerror_report(QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
> + error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
> bs->device_name, "cow", version);
> ret = -ENOTSUP;
> goto fail;
> @@ -330,7 +330,6 @@ static int cow_create(const char *filename, QEMUOptionParameter *options,
> struct stat st;
> int64_t image_sectors = 0;
> const char *image_filename = NULL;
> - Error *local_err = NULL;
> int ret;
> BlockDriverState *cow_bs;
>
> @@ -344,18 +343,13 @@ static int cow_create(const char *filename, QEMUOptionParameter *options,
> options++;
> }
>
> - ret = bdrv_create_file(filename, options, &local_err);
> + ret = bdrv_create_file(filename, options, errp);
> if (ret < 0) {
> - qerror_report_err(local_err);
> - error_free(local_err);
> return ret;
> }
>
> - ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR,
> - &local_err);
> + ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR, errp);
> if (ret < 0) {
> - qerror_report_err(local_err);
> - error_free(local_err);
> return ret;
> }
>
> --
> 1.8.5.3
>
I'm never sure about choice of using local_err with error_propagate, or
straightly pass errp, but the logic still looks right to me, so,
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [Qemu-devel] [PATCH 09/20] cow: correctly propagate errors
2014-02-10 8:04 ` Fam Zheng
@ 2014-02-10 8:28 ` Paolo Bonzini
0 siblings, 0 replies; 48+ messages in thread
From: Paolo Bonzini @ 2014-02-10 8:28 UTC (permalink / raw)
To: Fam Zheng; +Cc: kwolf, qemu-devel, stefanha, mreitz
Il 10/02/2014 09:04, Fam Zheng ha scritto:
> I'm never sure about choice of using local_err with error_propagate, or
> straightly pass errp, but the logic still looks right to me, so,
Basically, never use error_is_set. If you do, you probably should have
used instead &local_err and error_propagate. If you are not using
error_is_set, it's fine to use errp directly with no error_propagate.
Paolo
> Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 48+ messages in thread
* [Qemu-devel] [PATCH 10/20] curl: correctly propagate errors
2014-02-09 9:48 [Qemu-devel] [PATCH 00/20] Improve bdrv_open error messages Paolo Bonzini
` (8 preceding siblings ...)
2014-02-09 9:48 ` [Qemu-devel] [PATCH 09/20] cow: " Paolo Bonzini
@ 2014-02-09 9:48 ` Paolo Bonzini
2014-02-10 8:07 ` Fam Zheng
2014-02-09 9:48 ` [Qemu-devel] [PATCH 11/20] qcow: " Paolo Bonzini
` (9 subsequent siblings)
19 siblings, 1 reply; 48+ messages in thread
From: Paolo Bonzini @ 2014-02-09 9:48 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, mreitz
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/curl.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/block/curl.c b/block/curl.c
index a807584..353bab1 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -456,30 +456,27 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
static int inited = 0;
if (flags & BDRV_O_RDWR) {
- qerror_report(ERROR_CLASS_GENERIC_ERROR,
- "curl block device does not support writes");
+ error_setg(errp, "curl block device does not support writes");
return -EROFS;
}
opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
qemu_opts_absorb_qdict(opts, options, &local_err);
if (error_is_set(&local_err)) {
- qerror_report_err(local_err);
- error_free(local_err);
+ error_propagate(errp, local_err);
goto out_noclean;
}
s->readahead_size = qemu_opt_get_size(opts, "readahead", READ_AHEAD_SIZE);
if ((s->readahead_size & 0x1ff) != 0) {
- fprintf(stderr, "HTTP_READAHEAD_SIZE %zd is not a multiple of 512\n",
- s->readahead_size);
+ error_setg(errp, "HTTP_READAHEAD_SIZE %zd is not a multiple of 512\n",
+ s->readahead_size);
goto out_noclean;
}
file = qemu_opt_get(opts, "url");
if (file == NULL) {
- qerror_report(ERROR_CLASS_GENERIC_ERROR, "curl block driver requires "
- "an 'url' option");
+ error_setg(errp, "curl block driver requires an 'url' option");
goto out_noclean;
}
--
1.8.5.3
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [Qemu-devel] [PATCH 10/20] curl: correctly propagate errors
2014-02-09 9:48 ` [Qemu-devel] [PATCH 10/20] curl: " Paolo Bonzini
@ 2014-02-10 8:07 ` Fam Zheng
0 siblings, 0 replies; 48+ messages in thread
From: Fam Zheng @ 2014-02-10 8:07 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, qemu-devel, stefanha, mreitz
On Sun, 02/09 10:48, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> block/curl.c | 13 +++++--------
> 1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/block/curl.c b/block/curl.c
> index a807584..353bab1 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -456,30 +456,27 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
> static int inited = 0;
>
> if (flags & BDRV_O_RDWR) {
> - qerror_report(ERROR_CLASS_GENERIC_ERROR,
> - "curl block device does not support writes");
> + error_setg(errp, "curl block device does not support writes");
> return -EROFS;
> }
>
> opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
> qemu_opts_absorb_qdict(opts, options, &local_err);
> if (error_is_set(&local_err)) {
> - qerror_report_err(local_err);
> - error_free(local_err);
> + error_propagate(errp, local_err);
> goto out_noclean;
> }
>
> s->readahead_size = qemu_opt_get_size(opts, "readahead", READ_AHEAD_SIZE);
> if ((s->readahead_size & 0x1ff) != 0) {
> - fprintf(stderr, "HTTP_READAHEAD_SIZE %zd is not a multiple of 512\n",
> - s->readahead_size);
> + error_setg(errp, "HTTP_READAHEAD_SIZE %zd is not a multiple of 512\n",
> + s->readahead_size);
Getting rid of the inconsistency of fprintf(stderr,...)'s are good!
> goto out_noclean;
> }
>
> file = qemu_opt_get(opts, "url");
> if (file == NULL) {
> - qerror_report(ERROR_CLASS_GENERIC_ERROR, "curl block driver requires "
> - "an 'url' option");
> + error_setg(errp, "curl block driver requires an 'url' option");
> goto out_noclean;
> }
>
> --
> 1.8.5.3
>
>
>
Thanks,
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 48+ messages in thread
* [Qemu-devel] [PATCH 11/20] qcow: correctly propagate errors
2014-02-09 9:48 [Qemu-devel] [PATCH 00/20] Improve bdrv_open error messages Paolo Bonzini
` (9 preceding siblings ...)
2014-02-09 9:48 ` [Qemu-devel] [PATCH 10/20] curl: " Paolo Bonzini
@ 2014-02-09 9:48 ` Paolo Bonzini
2014-02-10 8:09 ` Fam Zheng
2014-02-09 9:48 ` [Qemu-devel] [PATCH 12/20] qed: " Paolo Bonzini
` (8 subsequent siblings)
19 siblings, 1 reply; 48+ messages in thread
From: Paolo Bonzini @ 2014-02-09 9:48 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, mreitz
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/qcow.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/block/qcow.c b/block/qcow.c
index 948b0c5..23bc691 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -119,17 +119,19 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
if (header.version != QCOW_VERSION) {
char version[64];
snprintf(version, sizeof(version), "QCOW version %d", header.version);
- qerror_report(QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
- bs->device_name, "qcow", version);
+ error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
+ bs->device_name, "qcow", version);
ret = -ENOTSUP;
goto fail;
}
if (header.size <= 1 || header.cluster_bits < 9) {
+ error_setg(errp, "invalid value in qcow header\n");
ret = -EINVAL;
goto fail;
}
if (header.crypt_method > QCOW_CRYPT_AES) {
+ error_setg(errp, "invalid encryption method in qcow header\n");
ret = -EINVAL;
goto fail;
}
@@ -668,7 +670,6 @@ static int qcow_create(const char *filename, QEMUOptionParameter *options,
int64_t total_size = 0;
const char *backing_file = NULL;
int flags = 0;
- Error *local_err = NULL;
int ret;
BlockDriverState *qcow_bs;
@@ -684,18 +685,13 @@ static int qcow_create(const char *filename, QEMUOptionParameter *options,
options++;
}
- ret = bdrv_create_file(filename, options, &local_err);
+ ret = bdrv_create_file(filename, options, errp);
if (ret < 0) {
- qerror_report_err(local_err);
- error_free(local_err);
return ret;
}
- ret = bdrv_file_open(&qcow_bs, filename, NULL, NULL, BDRV_O_RDWR,
- &local_err);
+ ret = bdrv_file_open(&qcow_bs, filename, NULL, NULL, BDRV_O_RDWR, errp);
if (ret < 0) {
- qerror_report_err(local_err);
- error_free(local_err);
return ret;
}
--
1.8.5.3
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [Qemu-devel] [PATCH 11/20] qcow: correctly propagate errors
2014-02-09 9:48 ` [Qemu-devel] [PATCH 11/20] qcow: " Paolo Bonzini
@ 2014-02-10 8:09 ` Fam Zheng
2014-02-10 8:28 ` Paolo Bonzini
0 siblings, 1 reply; 48+ messages in thread
From: Fam Zheng @ 2014-02-10 8:09 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, qemu-devel, stefanha, mreitz
On Sun, 02/09 10:48, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> block/qcow.c | 16 ++++++----------
> 1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/block/qcow.c b/block/qcow.c
> index 948b0c5..23bc691 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -119,17 +119,19 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
> if (header.version != QCOW_VERSION) {
> char version[64];
> snprintf(version, sizeof(version), "QCOW version %d", header.version);
> - qerror_report(QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
> - bs->device_name, "qcow", version);
> + error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
> + bs->device_name, "qcow", version);
> ret = -ENOTSUP;
> goto fail;
> }
>
> if (header.size <= 1 || header.cluster_bits < 9) {
> + error_setg(errp, "invalid value in qcow header\n");
The convention is not adding "\n" in the end of error messages.
> ret = -EINVAL;
> goto fail;
> }
> if (header.crypt_method > QCOW_CRYPT_AES) {
> + error_setg(errp, "invalid encryption method in qcow header\n");
Same here.
Thanks,
Fam
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [Qemu-devel] [PATCH 11/20] qcow: correctly propagate errors
2014-02-10 8:09 ` Fam Zheng
@ 2014-02-10 8:28 ` Paolo Bonzini
0 siblings, 0 replies; 48+ messages in thread
From: Paolo Bonzini @ 2014-02-10 8:28 UTC (permalink / raw)
To: Fam Zheng; +Cc: kwolf, qemu-devel, stefanha, mreitz
Il 10/02/2014 09:09, Fam Zheng ha scritto:
> On Sun, 02/09 10:48, Paolo Bonzini wrote:
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>> block/qcow.c | 16 ++++++----------
>> 1 file changed, 6 insertions(+), 10 deletions(-)
>>
>> diff --git a/block/qcow.c b/block/qcow.c
>> index 948b0c5..23bc691 100644
>> --- a/block/qcow.c
>> +++ b/block/qcow.c
>> @@ -119,17 +119,19 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
>> if (header.version != QCOW_VERSION) {
>> char version[64];
>> snprintf(version, sizeof(version), "QCOW version %d", header.version);
>> - qerror_report(QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
>> - bs->device_name, "qcow", version);
>> + error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
>> + bs->device_name, "qcow", version);
>> ret = -ENOTSUP;
>> goto fail;
>> }
>>
>> if (header.size <= 1 || header.cluster_bits < 9) {
>> + error_setg(errp, "invalid value in qcow header\n");
>
> The convention is not adding "\n" in the end of error messages.
>
>> ret = -EINVAL;
>> goto fail;
>> }
>> if (header.crypt_method > QCOW_CRYPT_AES) {
>> + error_setg(errp, "invalid encryption method in qcow header\n");
>
> Same here.
Oops, I'll fix all of them.
Paolo
^ permalink raw reply [flat|nested] 48+ messages in thread
* [Qemu-devel] [PATCH 12/20] qed: correctly propagate errors
2014-02-09 9:48 [Qemu-devel] [PATCH 00/20] Improve bdrv_open error messages Paolo Bonzini
` (10 preceding siblings ...)
2014-02-09 9:48 ` [Qemu-devel] [PATCH 11/20] qcow: " Paolo Bonzini
@ 2014-02-09 9:48 ` Paolo Bonzini
2014-02-10 8:11 ` Fam Zheng
2014-02-09 9:48 ` [Qemu-devel] [PATCH 13/20] vhdx: " Paolo Bonzini
` (7 subsequent siblings)
19 siblings, 1 reply; 48+ messages in thread
From: Paolo Bonzini @ 2014-02-09 9:48 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, mreitz
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/qed.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/block/qed.c b/block/qed.c
index 694e6e2..59bdd58 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -398,7 +398,7 @@ static int bdrv_qed_open(BlockDriverState *bs, QDict *options, int flags,
char buf[64];
snprintf(buf, sizeof(buf), "%" PRIx64,
s->header.features & ~QED_FEATURE_MASK);
- qerror_report(QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
+ error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
bs->device_name, "QED", buf);
return -ENOTSUP;
}
@@ -545,7 +545,8 @@ static void bdrv_qed_close(BlockDriverState *bs)
static int qed_create(const char *filename, uint32_t cluster_size,
uint64_t image_size, uint32_t table_size,
- const char *backing_file, const char *backing_fmt)
+ const char *backing_file, const char *backing_fmt,
+ Error **errp)
{
QEDHeader header = {
.magic = QED_MAGIC,
@@ -560,22 +561,17 @@ static int qed_create(const char *filename, uint32_t cluster_size,
QEDHeader le_header;
uint8_t *l1_table = NULL;
size_t l1_size = header.cluster_size * header.table_size;
- Error *local_err = NULL;
int ret = 0;
BlockDriverState *bs = NULL;
- ret = bdrv_create_file(filename, NULL, &local_err);
+ ret = bdrv_create_file(filename, NULL, errp);
if (ret < 0) {
- qerror_report_err(local_err);
- error_free(local_err);
return ret;
}
ret = bdrv_file_open(&bs, filename, NULL, NULL,
- BDRV_O_RDWR | BDRV_O_CACHE_WB, &local_err);
+ BDRV_O_RDWR | BDRV_O_CACHE_WB, errp);
if (ret < 0) {
- qerror_report_err(local_err);
- error_free(local_err);
return ret;
}
@@ -665,7 +661,7 @@ static int bdrv_qed_create(const char *filename, QEMUOptionParameter *options,
}
return qed_create(filename, cluster_size, image_size, table_size,
- backing_file, backing_fmt);
+ backing_file, backing_fmt, errp);
}
typedef struct {
--
1.8.5.3
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [Qemu-devel] [PATCH 12/20] qed: correctly propagate errors
2014-02-09 9:48 ` [Qemu-devel] [PATCH 12/20] qed: " Paolo Bonzini
@ 2014-02-10 8:11 ` Fam Zheng
0 siblings, 0 replies; 48+ messages in thread
From: Fam Zheng @ 2014-02-10 8:11 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, qemu-devel, stefanha, mreitz
On Sun, 02/09 10:48, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> block/qed.c | 16 ++++++----------
> 1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/block/qed.c b/block/qed.c
> index 694e6e2..59bdd58 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -398,7 +398,7 @@ static int bdrv_qed_open(BlockDriverState *bs, QDict *options, int flags,
> char buf[64];
> snprintf(buf, sizeof(buf), "%" PRIx64,
> s->header.features & ~QED_FEATURE_MASK);
> - qerror_report(QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
> + error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
> bs->device_name, "QED", buf);
> return -ENOTSUP;
> }
> @@ -545,7 +545,8 @@ static void bdrv_qed_close(BlockDriverState *bs)
>
> static int qed_create(const char *filename, uint32_t cluster_size,
> uint64_t image_size, uint32_t table_size,
> - const char *backing_file, const char *backing_fmt)
> + const char *backing_file, const char *backing_fmt,
> + Error **errp)
> {
> QEDHeader header = {
> .magic = QED_MAGIC,
> @@ -560,22 +561,17 @@ static int qed_create(const char *filename, uint32_t cluster_size,
> QEDHeader le_header;
> uint8_t *l1_table = NULL;
> size_t l1_size = header.cluster_size * header.table_size;
> - Error *local_err = NULL;
> int ret = 0;
> BlockDriverState *bs = NULL;
>
> - ret = bdrv_create_file(filename, NULL, &local_err);
> + ret = bdrv_create_file(filename, NULL, errp);
> if (ret < 0) {
> - qerror_report_err(local_err);
> - error_free(local_err);
> return ret;
> }
>
> ret = bdrv_file_open(&bs, filename, NULL, NULL,
> - BDRV_O_RDWR | BDRV_O_CACHE_WB, &local_err);
> + BDRV_O_RDWR | BDRV_O_CACHE_WB, errp);
> if (ret < 0) {
> - qerror_report_err(local_err);
> - error_free(local_err);
> return ret;
> }
>
> @@ -665,7 +661,7 @@ static int bdrv_qed_create(const char *filename, QEMUOptionParameter *options,
> }
>
> return qed_create(filename, cluster_size, image_size, table_size,
> - backing_file, backing_fmt);
> + backing_file, backing_fmt, errp);
> }
>
> typedef struct {
> --
> 1.8.5.3
>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 48+ messages in thread
* [Qemu-devel] [PATCH 13/20] vhdx: correctly propagate errors
2014-02-09 9:48 [Qemu-devel] [PATCH 00/20] Improve bdrv_open error messages Paolo Bonzini
` (11 preceding siblings ...)
2014-02-09 9:48 ` [Qemu-devel] [PATCH 12/20] qed: " Paolo Bonzini
@ 2014-02-09 9:48 ` Paolo Bonzini
2014-02-10 8:15 ` Fam Zheng
2014-02-09 9:48 ` [Qemu-devel] [PATCH 14/20] vvfat: " Paolo Bonzini
` (6 subsequent siblings)
19 siblings, 1 reply; 48+ messages in thread
From: Paolo Bonzini @ 2014-02-09 9:48 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, mreitz
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/vhdx.c | 21 +++++++++------------
1 file changed, 9 insertions(+), 12 deletions(-)
diff --git a/block/vhdx.c b/block/vhdx.c
index 9ee0a61..de1a80a 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -402,9 +402,10 @@ int vhdx_update_headers(BlockDriverState *bs, BDRVVHDXState *s,
}
/* opens the specified header block from the VHDX file header section */
-static int vhdx_parse_header(BlockDriverState *bs, BDRVVHDXState *s)
+static void vhdx_parse_header(BlockDriverState *bs, BDRVVHDXState *s,
+ Error **errp)
{
- int ret = 0;
+ int ret;
VHDXHeader *header1;
VHDXHeader *header2;
bool h1_valid = false;
@@ -462,7 +463,6 @@ static int vhdx_parse_header(BlockDriverState *bs, BDRVVHDXState *s)
} else if (!h1_valid && h2_valid) {
s->curr_header = 1;
} else if (!h1_valid && !h2_valid) {
- ret = -EINVAL;
goto fail;
} else {
/* If both headers are valid, then we choose the active one by the
@@ -473,27 +473,22 @@ static int vhdx_parse_header(BlockDriverState *bs, BDRVVHDXState *s)
} else if (h2_seq > h1_seq) {
s->curr_header = 1;
} else {
- ret = -EINVAL;
goto fail;
}
}
vhdx_region_register(s, s->headers[s->curr_header]->log_offset,
s->headers[s->curr_header]->log_length);
-
- ret = 0;
-
goto exit;
fail:
- qerror_report(ERROR_CLASS_GENERIC_ERROR, "No valid VHDX header found");
+ error_setg_errno(errp, -ret, "No valid VHDX header found");
qemu_vfree(header1);
qemu_vfree(header2);
s->headers[0] = NULL;
s->headers[1] = NULL;
exit:
qemu_vfree(buffer);
- return ret;
}
@@ -878,7 +873,7 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags,
int ret = 0;
uint32_t i;
uint64_t signature;
-
+ Error *local_err = NULL;
s->bat = NULL;
s->first_visible_write = true;
@@ -901,8 +896,10 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags,
* header update */
vhdx_guid_generate(&s->session_guid);
- ret = vhdx_parse_header(bs, s);
- if (ret < 0) {
+ vhdx_parse_header(bs, s, &local_err);
+ if (local_err != NULL) {
+ error_propagate(errp, local_err);
+ ret = -EINVAL;
goto fail;
}
--
1.8.5.3
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [Qemu-devel] [PATCH 13/20] vhdx: correctly propagate errors
2014-02-09 9:48 ` [Qemu-devel] [PATCH 13/20] vhdx: " Paolo Bonzini
@ 2014-02-10 8:15 ` Fam Zheng
0 siblings, 0 replies; 48+ messages in thread
From: Fam Zheng @ 2014-02-10 8:15 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, qemu-devel, stefanha, mreitz
On Sun, 02/09 10:48, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> block/vhdx.c | 21 +++++++++------------
> 1 file changed, 9 insertions(+), 12 deletions(-)
>
> diff --git a/block/vhdx.c b/block/vhdx.c
> index 9ee0a61..de1a80a 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -402,9 +402,10 @@ int vhdx_update_headers(BlockDriverState *bs, BDRVVHDXState *s,
> }
>
> /* opens the specified header block from the VHDX file header section */
> -static int vhdx_parse_header(BlockDriverState *bs, BDRVVHDXState *s)
> +static void vhdx_parse_header(BlockDriverState *bs, BDRVVHDXState *s,
> + Error **errp)
> {
> - int ret = 0;
> + int ret;
> VHDXHeader *header1;
> VHDXHeader *header2;
> bool h1_valid = false;
> @@ -462,7 +463,6 @@ static int vhdx_parse_header(BlockDriverState *bs, BDRVVHDXState *s)
> } else if (!h1_valid && h2_valid) {
> s->curr_header = 1;
> } else if (!h1_valid && !h2_valid) {
> - ret = -EINVAL;
> goto fail;
> } else {
> /* If both headers are valid, then we choose the active one by the
> @@ -473,27 +473,22 @@ static int vhdx_parse_header(BlockDriverState *bs, BDRVVHDXState *s)
> } else if (h2_seq > h1_seq) {
> s->curr_header = 1;
> } else {
> - ret = -EINVAL;
> goto fail;
> }
> }
>
> vhdx_region_register(s, s->headers[s->curr_header]->log_offset,
> s->headers[s->curr_header]->log_length);
> -
> - ret = 0;
> -
> goto exit;
>
> fail:
> - qerror_report(ERROR_CLASS_GENERIC_ERROR, "No valid VHDX header found");
> + error_setg_errno(errp, -ret, "No valid VHDX header found");
> qemu_vfree(header1);
> qemu_vfree(header2);
> s->headers[0] = NULL;
> s->headers[1] = NULL;
> exit:
> qemu_vfree(buffer);
> - return ret;
> }
>
>
> @@ -878,7 +873,7 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags,
> int ret = 0;
> uint32_t i;
> uint64_t signature;
> -
> + Error *local_err = NULL;
>
> s->bat = NULL;
> s->first_visible_write = true;
> @@ -901,8 +896,10 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags,
> * header update */
> vhdx_guid_generate(&s->session_guid);
>
> - ret = vhdx_parse_header(bs, s);
> - if (ret < 0) {
> + vhdx_parse_header(bs, s, &local_err);
> + if (local_err != NULL) {
> + error_propagate(errp, local_err);
> + ret = -EINVAL;
> goto fail;
> }
>
> --
> 1.8.5.3
>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 48+ messages in thread
* [Qemu-devel] [PATCH 14/20] vvfat: correctly propagate errors
2014-02-09 9:48 [Qemu-devel] [PATCH 00/20] Improve bdrv_open error messages Paolo Bonzini
` (12 preceding siblings ...)
2014-02-09 9:48 ` [Qemu-devel] [PATCH 13/20] vhdx: " Paolo Bonzini
@ 2014-02-09 9:48 ` Paolo Bonzini
2014-02-10 8:16 ` Fam Zheng
2014-02-09 9:48 ` [Qemu-devel] [PATCH 15/20] vmdk: extract vmdk_read_desc Paolo Bonzini
` (5 subsequent siblings)
19 siblings, 1 reply; 48+ messages in thread
From: Paolo Bonzini @ 2014-02-09 9:48 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, mreitz
Before:
$ ./qemu-io-old
qemu-io-old> open -r -o driver=vvfat,fat-type=24,dir=i386-softmmu
Valid FAT types are only 12, 16 and 32
qemu-io-old: can't open device (null): Could not open image: Invalid argument
After:
$ ./qemu-io
qemu-io> open -r -o driver=vvfat,fat-type=24,dir=i386-softmmu
qemu-io: can't open device (null): Valid FAT types are only 12, 16 and 32
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/vvfat.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/block/vvfat.c b/block/vvfat.c
index 664941c..7c3521a 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1086,16 +1086,14 @@ DLOG(if (stderr == NULL) {
opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
qemu_opts_absorb_qdict(opts, options, &local_err);
if (error_is_set(&local_err)) {
- qerror_report_err(local_err);
- error_free(local_err);
+ error_propagate(errp, local_err);
ret = -EINVAL;
goto fail;
}
dirname = qemu_opt_get(opts, "dir");
if (!dirname) {
- qerror_report(ERROR_CLASS_GENERIC_ERROR, "vvfat block driver requires "
- "a 'dir' option");
+ error_setg(errp, "vvfat block driver requires a 'dir' option");
ret = -EINVAL;
goto fail;
}
@@ -1135,8 +1133,7 @@ DLOG(if (stderr == NULL) {
case 12:
break;
default:
- qerror_report(ERROR_CLASS_GENERIC_ERROR, "Valid FAT types are only "
- "12, 16 and 32");
+ error_setg(errp, "Valid FAT types are only 12, 16 and 32");
ret = -EINVAL;
goto fail;
}
--
1.8.5.3
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [Qemu-devel] [PATCH 14/20] vvfat: correctly propagate errors
2014-02-09 9:48 ` [Qemu-devel] [PATCH 14/20] vvfat: " Paolo Bonzini
@ 2014-02-10 8:16 ` Fam Zheng
0 siblings, 0 replies; 48+ messages in thread
From: Fam Zheng @ 2014-02-10 8:16 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, qemu-devel, stefanha, mreitz
On Sun, 02/09 10:48, Paolo Bonzini wrote:
> Before:
> $ ./qemu-io-old
> qemu-io-old> open -r -o driver=vvfat,fat-type=24,dir=i386-softmmu
> Valid FAT types are only 12, 16 and 32
> qemu-io-old: can't open device (null): Could not open image: Invalid argument
>
> After:
> $ ./qemu-io
> qemu-io> open -r -o driver=vvfat,fat-type=24,dir=i386-softmmu
> qemu-io: can't open device (null): Valid FAT types are only 12, 16 and 32
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> block/vvfat.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/block/vvfat.c b/block/vvfat.c
> index 664941c..7c3521a 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -1086,16 +1086,14 @@ DLOG(if (stderr == NULL) {
> opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
> qemu_opts_absorb_qdict(opts, options, &local_err);
> if (error_is_set(&local_err)) {
> - qerror_report_err(local_err);
> - error_free(local_err);
> + error_propagate(errp, local_err);
> ret = -EINVAL;
> goto fail;
> }
>
> dirname = qemu_opt_get(opts, "dir");
> if (!dirname) {
> - qerror_report(ERROR_CLASS_GENERIC_ERROR, "vvfat block driver requires "
> - "a 'dir' option");
> + error_setg(errp, "vvfat block driver requires a 'dir' option");
> ret = -EINVAL;
> goto fail;
> }
> @@ -1135,8 +1133,7 @@ DLOG(if (stderr == NULL) {
> case 12:
> break;
> default:
> - qerror_report(ERROR_CLASS_GENERIC_ERROR, "Valid FAT types are only "
> - "12, 16 and 32");
> + error_setg(errp, "Valid FAT types are only 12, 16 and 32");
> ret = -EINVAL;
> goto fail;
> }
> --
> 1.8.5.3
>
>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 48+ messages in thread
* [Qemu-devel] [PATCH 15/20] vmdk: extract vmdk_read_desc
2014-02-09 9:48 [Qemu-devel] [PATCH 00/20] Improve bdrv_open error messages Paolo Bonzini
` (13 preceding siblings ...)
2014-02-09 9:48 ` [Qemu-devel] [PATCH 14/20] vvfat: " Paolo Bonzini
@ 2014-02-09 9:48 ` Paolo Bonzini
2014-02-10 8:28 ` Fam Zheng
2014-02-09 9:48 ` [Qemu-devel] [PATCH 16/20] vmdk: push vmdk_read_desc up to caller Paolo Bonzini
` (4 subsequent siblings)
19 siblings, 1 reply; 48+ messages in thread
From: Paolo Bonzini @ 2014-02-09 9:48 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, mreitz
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/vmdk.c | 40 +++++++++++++++++++++++++++++-----------
1 file changed, 29 insertions(+), 11 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index 99ca60f..58f4c34 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -529,6 +529,31 @@ static int vmdk_open_vmfs_sparse(BlockDriverState *bs,
static int vmdk_open_desc_file(BlockDriverState *bs, int flags,
uint64_t desc_offset, Error **errp);
+static char *vmdk_read_desc(BlockDriverState *file, uint64_t desc_offset,
+ Error **errp)
+{
+ int64_t size;
+ char *buf;
+ int ret;
+
+ size = bdrv_getlength(file);
+ if (size < 0) {
+ error_setg_errno(errp, -size, "Could not access file");
+ return NULL;
+ }
+
+ size = MIN(size, 1 << 20); /* avoid unbounded allocation */
+ buf = g_malloc0(size + 1);
+
+ ret = bdrv_pread(file, desc_offset, buf, size);
+ if (ret < 0) {
+ error_setg_errno(errp, -ret, "Could not read from file");
+ return NULL;
+ }
+
+ return buf;
+}
+
static int vmdk_open_vmdk4(BlockDriverState *bs,
BlockDriverState *file,
int flags, Error **errp)
@@ -822,23 +847,16 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags,
uint64_t desc_offset, Error **errp)
{
int ret;
- char *buf = NULL;
+ char *buf;
char ct[128];
BDRVVmdkState *s = bs->opaque;
- int64_t size;
- size = bdrv_getlength(bs->file);
- if (size < 0) {
+ buf = vmdk_read_desc(bs->file, desc_offset, errp);
+ if (!buf) {
return -EINVAL;
- }
-
- size = MIN(size, 1 << 20); /* avoid unbounded allocation */
- buf = g_malloc0(size + 1);
-
- ret = bdrv_pread(bs->file, desc_offset, buf, size);
- if (ret < 0) {
goto exit;
}
+
if (vmdk_parse_description(buf, "createType", ct, sizeof(ct))) {
ret = -EMEDIUMTYPE;
goto exit;
--
1.8.5.3
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [Qemu-devel] [PATCH 15/20] vmdk: extract vmdk_read_desc
2014-02-09 9:48 ` [Qemu-devel] [PATCH 15/20] vmdk: extract vmdk_read_desc Paolo Bonzini
@ 2014-02-10 8:28 ` Fam Zheng
0 siblings, 0 replies; 48+ messages in thread
From: Fam Zheng @ 2014-02-10 8:28 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, qemu-devel, stefanha, mreitz
On Sun, 02/09 10:48, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> block/vmdk.c | 40 +++++++++++++++++++++++++++++-----------
> 1 file changed, 29 insertions(+), 11 deletions(-)
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 99ca60f..58f4c34 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -529,6 +529,31 @@ static int vmdk_open_vmfs_sparse(BlockDriverState *bs,
> static int vmdk_open_desc_file(BlockDriverState *bs, int flags,
> uint64_t desc_offset, Error **errp);
>
> +static char *vmdk_read_desc(BlockDriverState *file, uint64_t desc_offset,
> + Error **errp)
> +{
> + int64_t size;
> + char *buf;
> + int ret;
> +
> + size = bdrv_getlength(file);
> + if (size < 0) {
> + error_setg_errno(errp, -size, "Could not access file");
> + return NULL;
> + }
> +
> + size = MIN(size, 1 << 20); /* avoid unbounded allocation */
> + buf = g_malloc0(size + 1);
> +
> + ret = bdrv_pread(file, desc_offset, buf, size);
> + if (ret < 0) {
> + error_setg_errno(errp, -ret, "Could not read from file");
buf is leaked here.
Fam
> + return NULL;
> + }
> +
> + return buf;
> +}
> +
> static int vmdk_open_vmdk4(BlockDriverState *bs,
> BlockDriverState *file,
> int flags, Error **errp)
> @@ -822,23 +847,16 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags,
> uint64_t desc_offset, Error **errp)
> {
> int ret;
> - char *buf = NULL;
> + char *buf;
> char ct[128];
> BDRVVmdkState *s = bs->opaque;
> - int64_t size;
>
> - size = bdrv_getlength(bs->file);
> - if (size < 0) {
> + buf = vmdk_read_desc(bs->file, desc_offset, errp);
> + if (!buf) {
> return -EINVAL;
> - }
> -
> - size = MIN(size, 1 << 20); /* avoid unbounded allocation */
> - buf = g_malloc0(size + 1);
> -
> - ret = bdrv_pread(bs->file, desc_offset, buf, size);
> - if (ret < 0) {
> goto exit;
> }
> +
> if (vmdk_parse_description(buf, "createType", ct, sizeof(ct))) {
> ret = -EMEDIUMTYPE;
> goto exit;
> --
> 1.8.5.3
>
>
^ permalink raw reply [flat|nested] 48+ messages in thread
* [Qemu-devel] [PATCH 16/20] vmdk: push vmdk_read_desc up to caller
2014-02-09 9:48 [Qemu-devel] [PATCH 00/20] Improve bdrv_open error messages Paolo Bonzini
` (14 preceding siblings ...)
2014-02-09 9:48 ` [Qemu-devel] [PATCH 15/20] vmdk: extract vmdk_read_desc Paolo Bonzini
@ 2014-02-09 9:48 ` Paolo Bonzini
2014-02-10 8:37 ` Fam Zheng
2014-02-09 9:48 ` [Qemu-devel] [PATCH 17/20] vmdk: do not try opening a file as both image and descriptor Paolo Bonzini
` (3 subsequent siblings)
19 siblings, 1 reply; 48+ messages in thread
From: Paolo Bonzini @ 2014-02-09 9:48 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, mreitz
Currently, we just try reading a VMDK file as both image and descriptor.
This makes it hard to choose which of the two attempts gave the best error.
We'll decide in advance if the file looks like an image or a descriptor,
and this patch is the first step to that end.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/vmdk.c | 54 ++++++++++++++++++++++++++++++------------------------
1 file changed, 30 insertions(+), 24 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index 58f4c34..74a0bac 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -526,8 +526,8 @@ static int vmdk_open_vmfs_sparse(BlockDriverState *bs,
return ret;
}
-static int vmdk_open_desc_file(BlockDriverState *bs, int flags,
- uint64_t desc_offset, Error **errp);
+static int vmdk_open_desc_file(BlockDriverState *bs, int flags, char *buf,
+ Error **errp);
static char *vmdk_read_desc(BlockDriverState *file, uint64_t desc_offset,
Error **errp)
@@ -575,7 +575,13 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
if (header.capacity == 0) {
uint64_t desc_offset = le64_to_cpu(header.desc_offset);
if (desc_offset) {
- return vmdk_open_desc_file(bs, flags, desc_offset << 9, errp);
+ char *buf = vmdk_read_desc(file, desc_offset << 9, errp);
+ if (!buf) {
+ return -EINVAL;
+ }
+ ret = vmdk_open_desc_file(bs, flags, buf, errp);
+ g_free(buf);
+ return ret;
}
}
@@ -726,16 +732,12 @@ static int vmdk_parse_description(const char *desc, const char *opt_name,
/* Open an extent file and append to bs array */
static int vmdk_open_sparse(BlockDriverState *bs,
- BlockDriverState *file,
- int flags, Error **errp)
+ BlockDriverState *file, int flags,
+ char *buf, Error **errp)
{
uint32_t magic;
- if (bdrv_pread(file, 0, &magic, sizeof(magic)) != sizeof(magic)) {
- return -EIO;
- }
-
- magic = be32_to_cpu(magic);
+ magic = ldl_be_p(buf);
switch (magic) {
case VMDK3_MAGIC:
return vmdk_open_vmfs_sparse(bs, file, flags, errp);
@@ -819,7 +821,12 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
extent->flat_start_offset = flat_offset << 9;
} else if (!strcmp(type, "SPARSE") || !strcmp(type, "VMFSSPARSE")) {
/* SPARSE extent and VMFSSPARSE extent are both "COWD" sparse file*/
- ret = vmdk_open_sparse(bs, extent_file, bs->open_flags, errp);
+ char *buf = vmdk_read_desc(extent_file, 0, errp);
+ if (!buf) {
+ ret = -EINVAL;
+ } else {
+ ret = vmdk_open_sparse(bs, extent_file, bs->open_flags, buf, errp);
+ }
if (ret) {
bdrv_unref(extent_file);
return ret;
@@ -843,20 +850,13 @@ next_line:
return 0;
}
-static int vmdk_open_desc_file(BlockDriverState *bs, int flags,
- uint64_t desc_offset, Error **errp)
+static int vmdk_open_desc_file(BlockDriverState *bs, int flags, char *buf,
+ Error **errp)
{
int ret;
- char *buf;
char ct[128];
BDRVVmdkState *s = bs->opaque;
- buf = vmdk_read_desc(bs->file, desc_offset, errp);
- if (!buf) {
- return -EINVAL;
- goto exit;
- }
-
if (vmdk_parse_description(buf, "createType", ct, sizeof(ct))) {
ret = -EMEDIUMTYPE;
goto exit;
@@ -874,20 +874,25 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags,
s->desc_offset = 0;
ret = vmdk_parse_extents(buf, bs, bs->file->filename, errp);
exit:
- g_free(buf);
return ret;
}
static int vmdk_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
+ char *buf = NULL;
int ret;
BDRVVmdkState *s = bs->opaque;
- if (vmdk_open_sparse(bs, bs->file, flags, errp) == 0) {
+ buf = vmdk_read_desc(bs->file, 0, errp);
+ if (!buf) {
+ return -EINVAL;
+ }
+
+ if (vmdk_open_sparse(bs, bs->file, flags, buf, errp) == 0) {
s->desc_offset = 0x200;
} else {
- ret = vmdk_open_desc_file(bs, flags, 0, errp);
+ ret = vmdk_open_desc_file(bs, flags, buf, errp);
if (ret) {
goto fail;
}
@@ -906,10 +911,11 @@ static int vmdk_open(BlockDriverState *bs, QDict *options, int flags,
QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
"vmdk", bs->device_name, "live migration");
migrate_add_blocker(s->migration_blocker);
-
+ g_free(buf);
return 0;
fail:
+ g_free(buf);
g_free(s->create_type);
s->create_type = NULL;
vmdk_free_extents(bs);
--
1.8.5.3
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [Qemu-devel] [PATCH 16/20] vmdk: push vmdk_read_desc up to caller
2014-02-09 9:48 ` [Qemu-devel] [PATCH 16/20] vmdk: push vmdk_read_desc up to caller Paolo Bonzini
@ 2014-02-10 8:37 ` Fam Zheng
0 siblings, 0 replies; 48+ messages in thread
From: Fam Zheng @ 2014-02-10 8:37 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, qemu-devel, stefanha, mreitz
On Sun, 02/09 10:48, Paolo Bonzini wrote:
> Currently, we just try reading a VMDK file as both image and descriptor.
> This makes it hard to choose which of the two attempts gave the best error.
> We'll decide in advance if the file looks like an image or a descriptor,
> and this patch is the first step to that end.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> block/vmdk.c | 54 ++++++++++++++++++++++++++++++------------------------
> 1 file changed, 30 insertions(+), 24 deletions(-)
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 58f4c34..74a0bac 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -526,8 +526,8 @@ static int vmdk_open_vmfs_sparse(BlockDriverState *bs,
> return ret;
> }
>
> -static int vmdk_open_desc_file(BlockDriverState *bs, int flags,
> - uint64_t desc_offset, Error **errp);
> +static int vmdk_open_desc_file(BlockDriverState *bs, int flags, char *buf,
> + Error **errp);
>
> static char *vmdk_read_desc(BlockDriverState *file, uint64_t desc_offset,
> Error **errp)
> @@ -575,7 +575,13 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
> if (header.capacity == 0) {
> uint64_t desc_offset = le64_to_cpu(header.desc_offset);
> if (desc_offset) {
> - return vmdk_open_desc_file(bs, flags, desc_offset << 9, errp);
> + char *buf = vmdk_read_desc(file, desc_offset << 9, errp);
> + if (!buf) {
> + return -EINVAL;
> + }
> + ret = vmdk_open_desc_file(bs, flags, buf, errp);
> + g_free(buf);
> + return ret;
> }
> }
>
> @@ -726,16 +732,12 @@ static int vmdk_parse_description(const char *desc, const char *opt_name,
>
> /* Open an extent file and append to bs array */
> static int vmdk_open_sparse(BlockDriverState *bs,
> - BlockDriverState *file,
> - int flags, Error **errp)
> + BlockDriverState *file, int flags,
> + char *buf, Error **errp)
> {
> uint32_t magic;
>
> - if (bdrv_pread(file, 0, &magic, sizeof(magic)) != sizeof(magic)) {
> - return -EIO;
> - }
> -
> - magic = be32_to_cpu(magic);
> + magic = ldl_be_p(buf);
> switch (magic) {
> case VMDK3_MAGIC:
> return vmdk_open_vmfs_sparse(bs, file, flags, errp);
> @@ -819,7 +821,12 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
> extent->flat_start_offset = flat_offset << 9;
> } else if (!strcmp(type, "SPARSE") || !strcmp(type, "VMFSSPARSE")) {
> /* SPARSE extent and VMFSSPARSE extent are both "COWD" sparse file*/
> - ret = vmdk_open_sparse(bs, extent_file, bs->open_flags, errp);
> + char *buf = vmdk_read_desc(extent_file, 0, errp);
> + if (!buf) {
> + ret = -EINVAL;
> + } else {
> + ret = vmdk_open_sparse(bs, extent_file, bs->open_flags, buf, errp);
> + }
> if (ret) {
Is buf leaked here when vmdk_open_sparse fails?
Fam
> bdrv_unref(extent_file);
> return ret;
^ permalink raw reply [flat|nested] 48+ messages in thread
* [Qemu-devel] [PATCH 17/20] vmdk: do not try opening a file as both image and descriptor
2014-02-09 9:48 [Qemu-devel] [PATCH 00/20] Improve bdrv_open error messages Paolo Bonzini
` (15 preceding siblings ...)
2014-02-09 9:48 ` [Qemu-devel] [PATCH 16/20] vmdk: push vmdk_read_desc up to caller Paolo Bonzini
@ 2014-02-09 9:48 ` Paolo Bonzini
2014-02-10 8:40 ` Fam Zheng
2014-02-09 9:48 ` [Qemu-devel] [PATCH 18/20] vmdk: correctly propagate errors Paolo Bonzini
` (2 subsequent siblings)
19 siblings, 1 reply; 48+ messages in thread
From: Paolo Bonzini @ 2014-02-09 9:48 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, mreitz
This prepares for propagating errors from vmdk_open_sparse and
vmdk_open_desc_file up to the caller of vmdk_open.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/vmdk.c | 22 +++++++++++++++-------
tests/qemu-iotests/059.out | 4 ++--
2 files changed, 17 insertions(+), 9 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index 74a0bac..750e632 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -883,20 +883,28 @@ static int vmdk_open(BlockDriverState *bs, QDict *options, int flags,
char *buf = NULL;
int ret;
BDRVVmdkState *s = bs->opaque;
+ uint32_t magic;
buf = vmdk_read_desc(bs->file, 0, errp);
if (!buf) {
return -EINVAL;
}
- if (vmdk_open_sparse(bs, bs->file, flags, buf, errp) == 0) {
- s->desc_offset = 0x200;
- } else {
- ret = vmdk_open_desc_file(bs, flags, buf, errp);
- if (ret) {
- goto fail;
- }
+ magic = ldl_be_p(buf);
+ switch (magic) {
+ case VMDK3_MAGIC:
+ case VMDK4_MAGIC:
+ ret = vmdk_open_sparse(bs, bs->file, flags, buf, errp);
+ s->desc_offset = 0x200;
+ break;
+ default:
+ ret = vmdk_open_desc_file(bs, flags, buf, errp);
+ break;
}
+ if (ret) {
+ goto fail;
+ }
+
/* try to open parent images, if exist */
ret = vmdk_parent_open(bs);
if (ret) {
diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
index 4ffeb54..4600670 100644
--- a/tests/qemu-iotests/059.out
+++ b/tests/qemu-iotests/059.out
@@ -8,7 +8,7 @@ no file open, try 'help open'
=== Testing too big L2 table size ===
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
L2 table size too big
-qemu-io: can't open device TEST_DIR/t.vmdk: Could not open 'TEST_DIR/t.vmdk': Wrong medium type
+qemu-io: can't open device TEST_DIR/t.vmdk: Could not open 'TEST_DIR/t.vmdk': Invalid argument
no file open, try 'help open'
=== Testing too big L1 table size ===
@@ -2046,7 +2046,7 @@ RW 12582912 VMFS "dummy.IMGFMT" 1
=== Testing truncated sparse ===
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=107374182400
qemu-img: File truncated, expecting at least 13172736 bytes
-qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Could not open 'TEST_DIR/t.IMGFMT': Wrong medium type
+qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Could not open 'TEST_DIR/t.IMGFMT': Invalid argument
=== Testing version 3 ===
image: TEST_DIR/iotest-version3.IMGFMT
--
1.8.5.3
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [Qemu-devel] [PATCH 17/20] vmdk: do not try opening a file as both image and descriptor
2014-02-09 9:48 ` [Qemu-devel] [PATCH 17/20] vmdk: do not try opening a file as both image and descriptor Paolo Bonzini
@ 2014-02-10 8:40 ` Fam Zheng
0 siblings, 0 replies; 48+ messages in thread
From: Fam Zheng @ 2014-02-10 8:40 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, qemu-devel, stefanha, mreitz
On Sun, 02/09 10:48, Paolo Bonzini wrote:
> This prepares for propagating errors from vmdk_open_sparse and
> vmdk_open_desc_file up to the caller of vmdk_open.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> block/vmdk.c | 22 +++++++++++++++-------
> tests/qemu-iotests/059.out | 4 ++--
> 2 files changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 74a0bac..750e632 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -883,20 +883,28 @@ static int vmdk_open(BlockDriverState *bs, QDict *options, int flags,
> char *buf = NULL;
> int ret;
> BDRVVmdkState *s = bs->opaque;
> + uint32_t magic;
>
> buf = vmdk_read_desc(bs->file, 0, errp);
> if (!buf) {
> return -EINVAL;
> }
>
> - if (vmdk_open_sparse(bs, bs->file, flags, buf, errp) == 0) {
> - s->desc_offset = 0x200;
> - } else {
> - ret = vmdk_open_desc_file(bs, flags, buf, errp);
> - if (ret) {
> - goto fail;
> - }
> + magic = ldl_be_p(buf);
> + switch (magic) {
> + case VMDK3_MAGIC:
> + case VMDK4_MAGIC:
> + ret = vmdk_open_sparse(bs, bs->file, flags, buf, errp);
> + s->desc_offset = 0x200;
> + break;
> + default:
> + ret = vmdk_open_desc_file(bs, flags, buf, errp);
> + break;
> }
> + if (ret) {
> + goto fail;
> + }
> +
> /* try to open parent images, if exist */
> ret = vmdk_parent_open(bs);
> if (ret) {
> diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
> index 4ffeb54..4600670 100644
> --- a/tests/qemu-iotests/059.out
> +++ b/tests/qemu-iotests/059.out
> @@ -8,7 +8,7 @@ no file open, try 'help open'
> === Testing too big L2 table size ===
> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> L2 table size too big
> -qemu-io: can't open device TEST_DIR/t.vmdk: Could not open 'TEST_DIR/t.vmdk': Wrong medium type
> +qemu-io: can't open device TEST_DIR/t.vmdk: Could not open 'TEST_DIR/t.vmdk': Invalid argument
> no file open, try 'help open'
>
> === Testing too big L1 table size ===
> @@ -2046,7 +2046,7 @@ RW 12582912 VMFS "dummy.IMGFMT" 1
> === Testing truncated sparse ===
> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=107374182400
> qemu-img: File truncated, expecting at least 13172736 bytes
> -qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Could not open 'TEST_DIR/t.IMGFMT': Wrong medium type
> +qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Could not open 'TEST_DIR/t.IMGFMT': Invalid argument
>
> === Testing version 3 ===
> image: TEST_DIR/iotest-version3.IMGFMT
> --
> 1.8.5.3
>
>
>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 48+ messages in thread
* [Qemu-devel] [PATCH 18/20] vmdk: correctly propagate errors
2014-02-09 9:48 [Qemu-devel] [PATCH 00/20] Improve bdrv_open error messages Paolo Bonzini
` (16 preceding siblings ...)
2014-02-09 9:48 ` [Qemu-devel] [PATCH 17/20] vmdk: do not try opening a file as both image and descriptor Paolo Bonzini
@ 2014-02-09 9:48 ` Paolo Bonzini
2014-02-10 8:41 ` Fam Zheng
2014-02-09 9:48 ` [Qemu-devel] [PATCH 19/20] block: do not abuse EMEDIUMTYPE Paolo Bonzini
2014-02-09 9:48 ` [Qemu-devel] [PATCH 20/20] vdi: say why an image is bad Paolo Bonzini
19 siblings, 1 reply; 48+ messages in thread
From: Paolo Bonzini @ 2014-02-09 9:48 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, mreitz
Now that we can return the "right" errors, use the Error** parameter
to pass them back instead of just printing them.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/vmdk.c | 11 ++++++-----
tests/qemu-iotests/059.out | 6 ++----
2 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index 750e632..f148164 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -571,6 +571,7 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
error_setg_errno(errp, -ret,
"Could not read header from file '%s'",
file->filename);
+ return -EINVAL;
}
if (header.capacity == 0) {
uint64_t desc_offset = le64_to_cpu(header.desc_offset);
@@ -640,8 +641,8 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
char buf[64];
snprintf(buf, sizeof(buf), "VMDK version %d",
le32_to_cpu(header.version));
- qerror_report(QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
- bs->device_name, "vmdk", buf);
+ error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
+ bs->device_name, "vmdk", buf);
return -ENOTSUP;
} else if (le32_to_cpu(header.version) == 3 && (flags & BDRV_O_RDWR)) {
/* VMware KB 2064959 explains that version 3 added support for
@@ -653,7 +654,7 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
}
if (le32_to_cpu(header.num_gtes_per_gt) > 512) {
- error_report("L2 table size too big");
+ error_setg(errp, "L2 table size too big");
return -EINVAL;
}
@@ -669,8 +670,8 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
}
if (bdrv_getlength(file) <
le64_to_cpu(header.grain_offset) * BDRV_SECTOR_SIZE) {
- error_report("File truncated, expecting at least %lld bytes",
- le64_to_cpu(header.grain_offset) * BDRV_SECTOR_SIZE);
+ error_setg(errp, "File truncated, expecting at least %lld bytes",
+ le64_to_cpu(header.grain_offset) * BDRV_SECTOR_SIZE);
return -EINVAL;
}
diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
index 4600670..3371c86 100644
--- a/tests/qemu-iotests/059.out
+++ b/tests/qemu-iotests/059.out
@@ -7,8 +7,7 @@ no file open, try 'help open'
=== Testing too big L2 table size ===
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-L2 table size too big
-qemu-io: can't open device TEST_DIR/t.vmdk: Could not open 'TEST_DIR/t.vmdk': Invalid argument
+qemu-io: can't open device TEST_DIR/t.vmdk: L2 table size too big
no file open, try 'help open'
=== Testing too big L1 table size ===
@@ -2045,8 +2044,7 @@ RW 12582912 VMFS "dummy.IMGFMT" 1
=== Testing truncated sparse ===
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=107374182400
-qemu-img: File truncated, expecting at least 13172736 bytes
-qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Could not open 'TEST_DIR/t.IMGFMT': Invalid argument
+qemu-img: Could not open 'TEST_DIR/t.IMGFMT': File truncated, expecting at least 13172736 bytes
=== Testing version 3 ===
image: TEST_DIR/iotest-version3.IMGFMT
--
1.8.5.3
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [Qemu-devel] [PATCH 18/20] vmdk: correctly propagate errors
2014-02-09 9:48 ` [Qemu-devel] [PATCH 18/20] vmdk: correctly propagate errors Paolo Bonzini
@ 2014-02-10 8:41 ` Fam Zheng
0 siblings, 0 replies; 48+ messages in thread
From: Fam Zheng @ 2014-02-10 8:41 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, qemu-devel, stefanha, mreitz
On Sun, 02/09 10:48, Paolo Bonzini wrote:
> Now that we can return the "right" errors, use the Error** parameter
> to pass them back instead of just printing them.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> block/vmdk.c | 11 ++++++-----
> tests/qemu-iotests/059.out | 6 ++----
> 2 files changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 750e632..f148164 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -571,6 +571,7 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
> error_setg_errno(errp, -ret,
> "Could not read header from file '%s'",
> file->filename);
> + return -EINVAL;
> }
> if (header.capacity == 0) {
> uint64_t desc_offset = le64_to_cpu(header.desc_offset);
> @@ -640,8 +641,8 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
> char buf[64];
> snprintf(buf, sizeof(buf), "VMDK version %d",
> le32_to_cpu(header.version));
> - qerror_report(QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
> - bs->device_name, "vmdk", buf);
> + error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
> + bs->device_name, "vmdk", buf);
> return -ENOTSUP;
> } else if (le32_to_cpu(header.version) == 3 && (flags & BDRV_O_RDWR)) {
> /* VMware KB 2064959 explains that version 3 added support for
> @@ -653,7 +654,7 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
> }
>
> if (le32_to_cpu(header.num_gtes_per_gt) > 512) {
> - error_report("L2 table size too big");
> + error_setg(errp, "L2 table size too big");
> return -EINVAL;
> }
>
> @@ -669,8 +670,8 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
> }
> if (bdrv_getlength(file) <
> le64_to_cpu(header.grain_offset) * BDRV_SECTOR_SIZE) {
> - error_report("File truncated, expecting at least %lld bytes",
> - le64_to_cpu(header.grain_offset) * BDRV_SECTOR_SIZE);
> + error_setg(errp, "File truncated, expecting at least %lld bytes",
> + le64_to_cpu(header.grain_offset) * BDRV_SECTOR_SIZE);
> return -EINVAL;
> }
>
> diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
> index 4600670..3371c86 100644
> --- a/tests/qemu-iotests/059.out
> +++ b/tests/qemu-iotests/059.out
> @@ -7,8 +7,7 @@ no file open, try 'help open'
>
> === Testing too big L2 table size ===
> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> -L2 table size too big
> -qemu-io: can't open device TEST_DIR/t.vmdk: Could not open 'TEST_DIR/t.vmdk': Invalid argument
> +qemu-io: can't open device TEST_DIR/t.vmdk: L2 table size too big
> no file open, try 'help open'
>
> === Testing too big L1 table size ===
> @@ -2045,8 +2044,7 @@ RW 12582912 VMFS "dummy.IMGFMT" 1
>
> === Testing truncated sparse ===
> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=107374182400
> -qemu-img: File truncated, expecting at least 13172736 bytes
> -qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Could not open 'TEST_DIR/t.IMGFMT': Invalid argument
> +qemu-img: Could not open 'TEST_DIR/t.IMGFMT': File truncated, expecting at least 13172736 bytes
>
> === Testing version 3 ===
> image: TEST_DIR/iotest-version3.IMGFMT
> --
> 1.8.5.3
>
>
>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 48+ messages in thread
* [Qemu-devel] [PATCH 19/20] block: do not abuse EMEDIUMTYPE
2014-02-09 9:48 [Qemu-devel] [PATCH 00/20] Improve bdrv_open error messages Paolo Bonzini
` (17 preceding siblings ...)
2014-02-09 9:48 ` [Qemu-devel] [PATCH 18/20] vmdk: correctly propagate errors Paolo Bonzini
@ 2014-02-09 9:48 ` Paolo Bonzini
2014-02-10 8:47 ` Fam Zheng
2014-02-09 9:48 ` [Qemu-devel] [PATCH 20/20] vdi: say why an image is bad Paolo Bonzini
19 siblings, 1 reply; 48+ messages in thread
From: Paolo Bonzini @ 2014-02-09 9:48 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, mreitz
Returning "Wrong medium type" for an image that does not have a valid
header is a bit weird. Improve the error by mentioning what format
was trying to open it.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/bochs.c | 3 ++-
block/cow.c | 3 ++-
block/parallels.c | 3 ++-
block/qcow.c | 3 ++-
block/qcow2.c | 2 +-
block/qed.c | 3 ++-
block/vdi.c | 4 ++--
block/vmdk.c | 6 ++++--
block/vpc.c | 3 ++-
9 files changed, 19 insertions(+), 11 deletions(-)
diff --git a/block/bochs.c b/block/bochs.c
index 51d9a90..f0f9a7e 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -129,7 +129,8 @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
strcmp(bochs.subtype, GROWING_TYPE) ||
((le32_to_cpu(bochs.version) != HEADER_VERSION) &&
(le32_to_cpu(bochs.version) != HEADER_V1))) {
- return -EMEDIUMTYPE;
+ error_setg(errp, "invalid Bochs image header\n");
+ return -EINVAL;
}
if (le32_to_cpu(bochs.version) == HEADER_V1) {
diff --git a/block/cow.c b/block/cow.c
index 43a2150..af7b824 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -74,7 +74,8 @@ static int cow_open(BlockDriverState *bs, QDict *options, int flags,
}
if (be32_to_cpu(cow_header.magic) != COW_MAGIC) {
- ret = -EMEDIUMTYPE;
+ error_setg(errp, "invalid COW image header\n");
+ ret = -EINVAL;
goto fail;
}
diff --git a/block/parallels.c b/block/parallels.c
index 2121e43..5c032f5 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -85,7 +85,8 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
if (memcmp(ph.magic, HEADER_MAGIC, 16) ||
(le32_to_cpu(ph.version) != HEADER_VERSION)) {
- ret = -EMEDIUMTYPE;
+ error_setg(errp, "invalid Parallels image header\n");
+ ret = -EINVAL;
goto fail;
}
diff --git a/block/qcow.c b/block/qcow.c
index 23bc691..292a314 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -113,7 +113,8 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
be64_to_cpus(&header.l1_table_offset);
if (header.magic != QCOW_MAGIC) {
- ret = -EMEDIUMTYPE;
+ error_setg(errp, "invalid qcow image header\n");
+ ret = -EINVAL;
goto fail;
}
if (header.version != QCOW_VERSION) {
diff --git a/block/qcow2.c b/block/qcow2.c
index 2da62b8..fa63d37 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -449,7 +449,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
if (header.magic != QCOW_MAGIC) {
error_setg(errp, "Image is not in qcow2 format");
- ret = -EMEDIUMTYPE;
+ ret = -EINVAL;
goto fail;
}
if (header.version < 2 || header.version > 3) {
diff --git a/block/qed.c b/block/qed.c
index 59bdd58..7efbc3b 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -391,7 +391,8 @@ static int bdrv_qed_open(BlockDriverState *bs, QDict *options, int flags,
qed_header_le_to_cpu(&le_header, &s->header);
if (s->header.magic != QED_MAGIC) {
- return -EMEDIUMTYPE;
+ error_setg(errp, "invalid QED image header\n");
+ return -EINVAL;
}
if (s->header.features & ~QED_FEATURE_MASK) {
/* image uses unsupported feature bits */
diff --git a/block/vdi.c b/block/vdi.c
index 2d7490f..68e152c 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -395,8 +395,8 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
}
if (header.signature != VDI_SIGNATURE) {
- logout("bad vdi signature %08x\n", header.signature);
- ret = -EMEDIUMTYPE;
+ error_setg(errp, "invalid VDI image (bad signature %08x)\n", header.signature);
+ ret = -EINVAL;
goto fail;
} else if (header.version != VDI_VERSION_1_1) {
logout("unsupported version %u.%u\n",
diff --git a/block/vmdk.c b/block/vmdk.c
index f148164..888a963 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -747,7 +747,8 @@ static int vmdk_open_sparse(BlockDriverState *bs,
return vmdk_open_vmdk4(bs, file, flags, errp);
break;
default:
- return -EMEDIUMTYPE;
+ error_setg(errp, "invalid VMDK image header\n");
+ return -EINVAL;
break;
}
}
@@ -859,7 +860,8 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags, char *buf,
BDRVVmdkState *s = bs->opaque;
if (vmdk_parse_description(buf, "createType", ct, sizeof(ct))) {
- ret = -EMEDIUMTYPE;
+ error_setg(errp, "invalid VMDK image descriptor\n");
+ ret = -EINVAL;
goto exit;
}
if (strcmp(ct, "monolithicFlat") &&
diff --git a/block/vpc.c b/block/vpc.c
index 1d326cb..238d91a 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -190,7 +190,8 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
goto fail;
}
if (strncmp(footer->creator, "conectix", 8)) {
- ret = -EMEDIUMTYPE;
+ error_setg(errp, "invalid VPC image\n");
+ ret = -EINVAL;
goto fail;
}
disk_type = VHD_FIXED;
--
1.8.5.3
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [Qemu-devel] [PATCH 19/20] block: do not abuse EMEDIUMTYPE
2014-02-09 9:48 ` [Qemu-devel] [PATCH 19/20] block: do not abuse EMEDIUMTYPE Paolo Bonzini
@ 2014-02-10 8:47 ` Fam Zheng
2014-02-10 9:01 ` Paolo Bonzini
0 siblings, 1 reply; 48+ messages in thread
From: Fam Zheng @ 2014-02-10 8:47 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, qemu-devel, stefanha, mreitz
On Sun, 02/09 10:48, Paolo Bonzini wrote:
> diff --git a/block/bochs.c b/block/bochs.c
> index 51d9a90..f0f9a7e 100644
> --- a/block/bochs.c
> +++ b/block/bochs.c
> @@ -129,7 +129,8 @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
> strcmp(bochs.subtype, GROWING_TYPE) ||
> ((le32_to_cpu(bochs.version) != HEADER_VERSION) &&
> (le32_to_cpu(bochs.version) != HEADER_V1))) {
> - return -EMEDIUMTYPE;
> + error_setg(errp, "invalid Bochs image header\n");
Ending "\n" is not necessary, including all following cases.
> + return -EINVAL;
> }
>
> if (le32_to_cpu(bochs.version) == HEADER_V1) {
<snip>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 2da62b8..fa63d37 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -449,7 +449,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
>
> if (header.magic != QCOW_MAGIC) {
> error_setg(errp, "Image is not in qcow2 format");
It might be good to have a consistent message pattern in qcow2 as others. Is it
worth adding a QERR_ error class for unexpected format magic?
Fam
> - ret = -EMEDIUMTYPE;
> + ret = -EINVAL;
> goto fail;
> }
> if (header.version < 2 || header.version > 3) {
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [Qemu-devel] [PATCH 19/20] block: do not abuse EMEDIUMTYPE
2014-02-10 8:47 ` Fam Zheng
@ 2014-02-10 9:01 ` Paolo Bonzini
0 siblings, 0 replies; 48+ messages in thread
From: Paolo Bonzini @ 2014-02-10 9:01 UTC (permalink / raw)
To: Fam Zheng; +Cc: kwolf, qemu-devel, stefanha, mreitz
Il 10/02/2014 09:47, Fam Zheng ha scritto:
>> > @@ -449,7 +449,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
>> >
>> > if (header.magic != QCOW_MAGIC) {
>> > error_setg(errp, "Image is not in qcow2 format");
> It might be good to have a consistent message pattern in qcow2 as others.
Right, I'll fix the others.
> Is it worth adding a QERR_ error class for unexpected format magic?
We're not adding QERR_ anymore, but if anyone has ideas I can do
something to improve the homogeneity of the errors.
Paolo
^ permalink raw reply [flat|nested] 48+ messages in thread
* [Qemu-devel] [PATCH 20/20] vdi: say why an image is bad
2014-02-09 9:48 [Qemu-devel] [PATCH 00/20] Improve bdrv_open error messages Paolo Bonzini
` (18 preceding siblings ...)
2014-02-09 9:48 ` [Qemu-devel] [PATCH 19/20] block: do not abuse EMEDIUMTYPE Paolo Bonzini
@ 2014-02-09 9:48 ` Paolo Bonzini
2014-02-10 8:53 ` Fam Zheng
19 siblings, 1 reply; 48+ messages in thread
From: Paolo Bonzini @ 2014-02-09 9:48 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, mreitz
Instead of just putting it in debugging output, we can now put the
value in an Error.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/vdi.c | 25 ++++++++++++++++---------
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/block/vdi.c b/block/vdi.c
index 68e152c..3859e49 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -399,39 +399,46 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
ret = -EINVAL;
goto fail;
} else if (header.version != VDI_VERSION_1_1) {
- logout("unsupported version %u.%u\n",
- header.version >> 16, header.version & 0xffff);
+ error_setg(errp, "unsupported VDI image (version %u.%u)\n",
+ header.version >> 16, header.version & 0xffff);
ret = -ENOTSUP;
goto fail;
} else if (header.offset_bmap % SECTOR_SIZE != 0) {
/* We only support block maps which start on a sector boundary. */
- logout("unsupported block map offset 0x%x B\n", header.offset_bmap);
+ error_setg(errp, "unsupported VDI image (unaligned block map offset 0x%x)\n",
+ header.offset_bmap);
ret = -ENOTSUP;
goto fail;
} else if (header.offset_data % SECTOR_SIZE != 0) {
/* We only support data blocks which start on a sector boundary. */
- logout("unsupported data offset 0x%x B\n", header.offset_data);
+ error_setg(errp, "unsupported VDI image (unaligned data offset 0x%x)\n",
+ header.offset_data);
ret = -ENOTSUP;
goto fail;
} else if (header.sector_size != SECTOR_SIZE) {
- logout("unsupported sector size %u B\n", header.sector_size);
+ error_setg(errp, "unsupported VDI image (sector size %u is not %u)\n",
+ header.sector_size, SECTOR_SIZE);
ret = -ENOTSUP;
goto fail;
} else if (header.block_size != 1 * MiB) {
- logout("unsupported block size %u B\n", header.block_size);
+ error_setg(errp, "unsupported VDI image (sector size %u is not %u)\n",
+ header.block_size, 1 * MiB);
ret = -ENOTSUP;
goto fail;
} else if (header.disk_size >
(uint64_t)header.blocks_in_image * header.block_size) {
- logout("unsupported disk size %" PRIu64 " B\n", header.disk_size);
+ error_setg(errp, "unsupported VDI image (disk size %" PRIu64 ", "
+ "image bitmap has room for %" PRIu64 ")\n",
+ header.disk_size,
+ (uint64_t)header.blocks_in_image * header.block_size);
ret = -ENOTSUP;
goto fail;
} else if (!uuid_is_null(header.uuid_link)) {
- logout("link uuid != 0, unsupported\n");
+ error_setg(errp, "unsupported VDI image (non-NULL link UUID)\n");
ret = -ENOTSUP;
goto fail;
} else if (!uuid_is_null(header.uuid_parent)) {
- logout("parent uuid != 0, unsupported\n");
+ error_setg(errp, "unsupported VDI image (non-NULL parent UUID)\n");
ret = -ENOTSUP;
goto fail;
}
--
1.8.5.3
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [Qemu-devel] [PATCH 20/20] vdi: say why an image is bad
2014-02-09 9:48 ` [Qemu-devel] [PATCH 20/20] vdi: say why an image is bad Paolo Bonzini
@ 2014-02-10 8:53 ` Fam Zheng
0 siblings, 0 replies; 48+ messages in thread
From: Fam Zheng @ 2014-02-10 8:53 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, qemu-devel, stefanha, mreitz
On Sun, 02/09 10:48, Paolo Bonzini wrote:
> Instead of just putting it in debugging output, we can now put the
> value in an Error.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> block/vdi.c | 25 ++++++++++++++++---------
> 1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/block/vdi.c b/block/vdi.c
> index 68e152c..3859e49 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -399,39 +399,46 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
> ret = -EINVAL;
> goto fail;
> } else if (header.version != VDI_VERSION_1_1) {
> - logout("unsupported version %u.%u\n",
> - header.version >> 16, header.version & 0xffff);
> + error_setg(errp, "unsupported VDI image (version %u.%u)\n",
> + header.version >> 16, header.version & 0xffff);
> ret = -ENOTSUP;
> goto fail;
> } else if (header.offset_bmap % SECTOR_SIZE != 0) {
> /* We only support block maps which start on a sector boundary. */
> - logout("unsupported block map offset 0x%x B\n", header.offset_bmap);
> + error_setg(errp, "unsupported VDI image (unaligned block map offset 0x%x)\n",
> + header.offset_bmap);
> ret = -ENOTSUP;
> goto fail;
> } else if (header.offset_data % SECTOR_SIZE != 0) {
> /* We only support data blocks which start on a sector boundary. */
> - logout("unsupported data offset 0x%x B\n", header.offset_data);
> + error_setg(errp, "unsupported VDI image (unaligned data offset 0x%x)\n",
> + header.offset_data);
> ret = -ENOTSUP;
> goto fail;
> } else if (header.sector_size != SECTOR_SIZE) {
> - logout("unsupported sector size %u B\n", header.sector_size);
> + error_setg(errp, "unsupported VDI image (sector size %u is not %u)\n",
> + header.sector_size, SECTOR_SIZE);
> ret = -ENOTSUP;
> goto fail;
> } else if (header.block_size != 1 * MiB) {
> - logout("unsupported block size %u B\n", header.block_size);
> + error_setg(errp, "unsupported VDI image (sector size %u is not %u)\n",
s/sector size/block size/
> + header.block_size, 1 * MiB);
> ret = -ENOTSUP;
> goto fail;
> } else if (header.disk_size >
> (uint64_t)header.blocks_in_image * header.block_size) {
> - logout("unsupported disk size %" PRIu64 " B\n", header.disk_size);
> + error_setg(errp, "unsupported VDI image (disk size %" PRIu64 ", "
> + "image bitmap has room for %" PRIu64 ")\n",
> + header.disk_size,
> + (uint64_t)header.blocks_in_image * header.block_size);
Improved, good!
> ret = -ENOTSUP;
> goto fail;
> } else if (!uuid_is_null(header.uuid_link)) {
> - logout("link uuid != 0, unsupported\n");
> + error_setg(errp, "unsupported VDI image (non-NULL link UUID)\n");
> ret = -ENOTSUP;
> goto fail;
> } else if (!uuid_is_null(header.uuid_parent)) {
> - logout("parent uuid != 0, unsupported\n");
> + error_setg(errp, "unsupported VDI image (non-NULL parent UUID)\n");
> ret = -ENOTSUP;
> goto fail;
> }
There are also unnecessary ending "\n" in the messages.
Fam
^ permalink raw reply [flat|nested] 48+ messages in thread