| Message ID | 1447224690-9743-19-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 11.11.2015 um 07:51 schrieb Eric Blake: > A few uses of error_set(ERROR_CLASS_GENERIC_ERROR) have snuck in > since c6bd8c706. Nuke them. > > Signed-off-by: Eric Blake <eblake@redhat.com> [...] > diff --git a/qom/object.c b/qom/object.c > index fc6e161..c0decb6 100644 > --- a/qom/object.c > +++ b/qom/object.c > @@ -1330,8 +1330,8 @@ static Object *object_resolve_link(Object *obj, const char *name, > target = object_resolve_path_type(path, target_type, &ambiguous); > > if (ambiguous) { > - error_set(errp, ERROR_CLASS_GENERIC_ERROR, > - "Path '%s' does not uniquely identify an object", path); > + error_setg(errp, "Path '%s' does not uniquely identify an object", > + path); > } else if (!target) { > target = object_resolve_path(path, &ambiguous); > if (target || ambiguous) { Acked-by: Andreas Färber <afaerber@suse.de> No idea why it's this way, maybe predated the function? Regards, Andreas
Eric Blake <eblake@redhat.com> writes: > A few uses of error_set(ERROR_CLASS_GENERIC_ERROR) have snuck in > since c6bd8c706. Nuke them. Doesn't really belong to this series, but that's okay. > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v11: new patch > --- > block.c | 3 +-- > docs/writing-qmp-commands.txt | 20 +++++++++----------- > hw/i386/pc.c | 2 +- > hw/net/rocker/rocker.c | 6 ++---- > hw/net/rocker/rocker_of_dpa.c | 12 ++++-------- > qom/object.c | 4 ++-- > 6 files changed, 19 insertions(+), 28 deletions(-) > > diff --git a/block.c b/block.c > index e9f40dc..53a978a 100644 > --- a/block.c > +++ b/block.c > @@ -1795,8 +1795,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue, > > ret = bdrv_flush(reopen_state->bs); > if (ret) { > - error_set(errp, ERROR_CLASS_GENERIC_ERROR, "Error (%s) flushing drive", > - strerror(-ret)); > + error_setg_errno(errp, -ret, "Error flushing drive"); > goto error; > } > > diff --git a/docs/writing-qmp-commands.txt b/docs/writing-qmp-commands.txt > index 8647cac..59aa77a 100644 > --- a/docs/writing-qmp-commands.txt > +++ b/docs/writing-qmp-commands.txt > @@ -210,7 +210,7 @@ if you don't see these strings, then something went wrong. > === Errors === > > QMP commands should use the error interface exported by the error.h header > -file. Basically, errors are set by calling the error_set() function. > +file. Basically, most errors are set by calling the error_setg() function. > > Let's say we don't accept the string "message" to contain the word "love". If > it does contain it, we want the "hello-world" command to return an error: > @@ -219,8 +219,7 @@ void qmp_hello_world(bool has_message, const char *message, Error **errp) > { > if (has_message) { > if (strstr(message, "love")) { > - error_set(errp, ERROR_CLASS_GENERIC_ERROR, > - "the word 'love' is not allowed"); > + error_setg(errp, "the word 'love' is not allowed"); > return; > } > printf("%s\n", message); > @@ -229,10 +228,8 @@ void qmp_hello_world(bool has_message, const char *message, Error **errp) > } > } > > -The first argument to the error_set() function is the Error pointer to pointer, > -which is passed to all QMP functions. The second argument is a ErrorClass > -value, which should be ERROR_CLASS_GENERIC_ERROR most of the time (more > -details about error classes are given below). The third argument is a human > +The first argument to the error_setg() function is the Error pointer > +to pointer, which is passed to all QMP functions. The next argument is a human > description of the error, this is a free-form printf-like string. > > Let's test the example above. Build qemu, run it as defined in the "Testing" > @@ -249,8 +246,9 @@ The QMP server's response should be: > } > } > > -As a general rule, all QMP errors should use ERROR_CLASS_GENERIC_ERROR. There > -are two exceptions to this rule: > +As a general rule, all QMP errors should use ERROR_CLASS_GENERIC_ERROR > +(done by default when using error_setg()). There are two exceptions to > +this rule: > > 1. A non-generic ErrorClass value exists* for the failure you want to report > (eg. DeviceNotFound) > @@ -259,8 +257,8 @@ are two exceptions to this rule: > want to report, hence you have to add a new ErrorClass value so that they > can check for it > > -If the failure you want to report doesn't fall in one of the two cases above, > -just report ERROR_CLASS_GENERIC_ERROR. > +If the failure you want to report falls into one of the two cases above, > +use error_set() with a second argument of an ErrorClass value. > > * All existing ErrorClass values are defined in the qapi-schema.json file > Thanks a lot for this doc update! > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 0cb8afd..dfb57a8 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1795,7 +1795,7 @@ static void pc_machine_set_max_ram_below_4g(Object *obj, Visitor *v, > return; > } > if (value > (1ULL << 32)) { > - error_set(&error, ERROR_CLASS_GENERIC_ERROR, > + error_setg(&error, > "Machine option 'max-ram-below-4g=%"PRIu64 > "' expects size less than or equal to 4G", value); Indentation is now off. Can tidy up in my tree. > error_propagate(errp, error); [Rest snipped, it looks good]
Am 11.11.2015 um 15:21 schrieb Markus Armbruster: > Eric Blake <eblake@redhat.com> writes: > >> A few uses of error_set(ERROR_CLASS_GENERIC_ERROR) have snuck in >> since c6bd8c706. Nuke them. > > Doesn't really belong to this series, but that's okay. It seemed to avoid a GENERIC_ERROR -> GENERICERROR change in the following patch, so it is related. Cheers, Andreas
On 11/11/2015 07:23 AM, Andreas Färber wrote: > Am 11.11.2015 um 15:21 schrieb Markus Armbruster: >> Eric Blake <eblake@redhat.com> writes: >> >>> A few uses of error_set(ERROR_CLASS_GENERIC_ERROR) have snuck in >>> since c6bd8c706. Nuke them. >> >> Doesn't really belong to this series, but that's okay. > > It seemed to avoid a GENERIC_ERROR -> GENERICERROR change in the > following patch, so it is related. Indeed, that's why it ended up here. But I'm also okay if we want to detach this one from the series and apply it now through the qerror tree for 2.5, as it includes a doc update and should be safe even during hard freeze.
On 11/11/2015 07:21 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> A few uses of error_set(ERROR_CLASS_GENERIC_ERROR) have snuck in >> since c6bd8c706. Nuke them. > > Doesn't really belong to this series, but that's okay. If you're going to modify this for 2.5 inclusion through your qerror tree, you may want to change the description to: A few uses of error_set(ERROR_CLASS_GENERIC_ERROR) were missed in c6bd8c706, or have snuck in since. Nuke them. >> +++ b/hw/i386/pc.c >> @@ -1795,7 +1795,7 @@ static void pc_machine_set_max_ram_below_4g(Object *obj, Visitor *v, >> return; >> } >> if (value > (1ULL << 32)) { >> - error_set(&error, ERROR_CLASS_GENERIC_ERROR, >> + error_setg(&error, >> "Machine option 'max-ram-below-4g=%"PRIu64 >> "' expects size less than or equal to 4G", value); > > Indentation is now off. Can tidy up in my tree. > >> error_propagate(errp, error); > [Rest snipped, it looks good] > There's also the question if we want to address the ErrorClass name munging of 19/28 by adding your idea of an aliasing typedef in error.h; if so, should I prepare a smaller patch series of both of those changes for consideration for 2.5?
Eric Blake <eblake@redhat.com> writes: > On 11/11/2015 07:21 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> A few uses of error_set(ERROR_CLASS_GENERIC_ERROR) have snuck in >>> since c6bd8c706. Nuke them. >> >> Doesn't really belong to this series, but that's okay. > > If you're going to modify this for 2.5 inclusion through your qerror > tree, you may want to change the description to: I'll try to get this into 2.5 mostly for the documentation update. > A few uses of error_set(ERROR_CLASS_GENERIC_ERROR) were missed in > c6bd8c706, or have snuck in since. Nuke them. Done. >>> +++ b/hw/i386/pc.c >>> @@ -1795,7 +1795,7 @@ static void pc_machine_set_max_ram_below_4g(Object *obj, Visitor *v, >>> return; >>> } >>> if (value > (1ULL << 32)) { >>> - error_set(&error, ERROR_CLASS_GENERIC_ERROR, >>> + error_setg(&error, >>> "Machine option 'max-ram-below-4g=%"PRIu64 >>> "' expects size less than or equal to 4G", value); >> >> Indentation is now off. Can tidy up in my tree. >> >>> error_propagate(errp, error); >> [Rest snipped, it looks good] >> > > There's also the question if we want to address the ErrorClass name > munging of 19/28 by adding your idea of an aliasing typedef in error.h; > if so, should I prepare a smaller patch series of both of those changes > for consideration for 2.5? Can safely wait for 2.6, can't it?
On 11/11/2015 10:31 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> On 11/11/2015 07:21 AM, Markus Armbruster wrote: >>> Eric Blake <eblake@redhat.com> writes: >>> >>>> A few uses of error_set(ERROR_CLASS_GENERIC_ERROR) have snuck in >>>> since c6bd8c706. Nuke them. >>> >>> Doesn't really belong to this series, but that's okay. >> >> If you're going to modify this for 2.5 inclusion through your qerror >> tree, you may want to change the description to: > > I'll try to get this into 2.5 mostly for the documentation update. > >> A few uses of error_set(ERROR_CLASS_GENERIC_ERROR) were missed in >> c6bd8c706, or have snuck in since. Nuke them. > > Done. Oh, and I missed your capitalization preferences in the title: qerror: More error_setg() usage >> There's also the question if we want to address the ErrorClass name >> munging of 19/28 by adding your idea of an aliasing typedef in error.h; >> if so, should I prepare a smaller patch series of both of those changes >> for consideration for 2.5? > > Can safely wait for 2.6, can't it? Yes. Just trying to make sure I'm focusing on 2.5 patches first.
diff --git a/block.c b/block.c index e9f40dc..53a978a 100644 --- a/block.c +++ b/block.c @@ -1795,8 +1795,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue, ret = bdrv_flush(reopen_state->bs); if (ret) { - error_set(errp, ERROR_CLASS_GENERIC_ERROR, "Error (%s) flushing drive", - strerror(-ret)); + error_setg_errno(errp, -ret, "Error flushing drive"); goto error; } diff --git a/docs/writing-qmp-commands.txt b/docs/writing-qmp-commands.txt index 8647cac..59aa77a 100644 --- a/docs/writing-qmp-commands.txt +++ b/docs/writing-qmp-commands.txt @@ -210,7 +210,7 @@ if you don't see these strings, then something went wrong. === Errors === QMP commands should use the error interface exported by the error.h header -file. Basically, errors are set by calling the error_set() function. +file. Basically, most errors are set by calling the error_setg() function. Let's say we don't accept the string "message" to contain the word "love". If it does contain it, we want the "hello-world" command to return an error: @@ -219,8 +219,7 @@ void qmp_hello_world(bool has_message, const char *message, Error **errp) { if (has_message) { if (strstr(message, "love")) { - error_set(errp, ERROR_CLASS_GENERIC_ERROR, - "the word 'love' is not allowed"); + error_setg(errp, "the word 'love' is not allowed"); return; } printf("%s\n", message); @@ -229,10 +228,8 @@ void qmp_hello_world(bool has_message, const char *message, Error **errp) } } -The first argument to the error_set() function is the Error pointer to pointer, -which is passed to all QMP functions. The second argument is a ErrorClass -value, which should be ERROR_CLASS_GENERIC_ERROR most of the time (more -details about error classes are given below). The third argument is a human +The first argument to the error_setg() function is the Error pointer +to pointer, which is passed to all QMP functions. The next argument is a human description of the error, this is a free-form printf-like string. Let's test the example above. Build qemu, run it as defined in the "Testing" @@ -249,8 +246,9 @@ The QMP server's response should be: } } -As a general rule, all QMP errors should use ERROR_CLASS_GENERIC_ERROR. There -are two exceptions to this rule: +As a general rule, all QMP errors should use ERROR_CLASS_GENERIC_ERROR +(done by default when using error_setg()). There are two exceptions to +this rule: 1. A non-generic ErrorClass value exists* for the failure you want to report (eg. DeviceNotFound) @@ -259,8 +257,8 @@ are two exceptions to this rule: want to report, hence you have to add a new ErrorClass value so that they can check for it -If the failure you want to report doesn't fall in one of the two cases above, -just report ERROR_CLASS_GENERIC_ERROR. +If the failure you want to report falls into one of the two cases above, +use error_set() with a second argument of an ErrorClass value. * All existing ErrorClass values are defined in the qapi-schema.json file diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 0cb8afd..dfb57a8 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1795,7 +1795,7 @@ static void pc_machine_set_max_ram_below_4g(Object *obj, Visitor *v, return; } if (value > (1ULL << 32)) { - error_set(&error, ERROR_CLASS_GENERIC_ERROR, + error_setg(&error, "Machine option 'max-ram-below-4g=%"PRIu64 "' expects size less than or equal to 4G", value); error_propagate(errp, error); diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c index bb6fdc3..c57f1a6 100644 --- a/hw/net/rocker/rocker.c +++ b/hw/net/rocker/rocker.c @@ -101,8 +101,7 @@ RockerSwitch *qmp_query_rocker(const char *name, Error **errp) r = rocker_find(name); if (!r) { - error_set(errp, ERROR_CLASS_GENERIC_ERROR, - "rocker %s not found", name); + error_setg(errp, "rocker %s not found", name); return NULL; } @@ -122,8 +121,7 @@ RockerPortList *qmp_query_rocker_ports(const char *name, Error **errp) r = rocker_find(name); if (!r) { - error_set(errp, ERROR_CLASS_GENERIC_ERROR, - "rocker %s not found", name); + error_setg(errp, "rocker %s not found", name); return NULL; } diff --git a/hw/net/rocker/rocker_of_dpa.c b/hw/net/rocker/rocker_of_dpa.c index 1ad2791..3cf1d61 100644 --- a/hw/net/rocker/rocker_of_dpa.c +++ b/hw/net/rocker/rocker_of_dpa.c @@ -2462,15 +2462,13 @@ RockerOfDpaFlowList *qmp_query_rocker_of_dpa_flows(const char *name, r = rocker_find(name); if (!r) { - error_set(errp, ERROR_CLASS_GENERIC_ERROR, - "rocker %s not found", name); + error_setg(errp, "rocker %s not found", name); return NULL; } w = rocker_get_world(r, ROCKER_WORLD_TYPE_OF_DPA); if (!w) { - error_set(errp, ERROR_CLASS_GENERIC_ERROR, - "rocker %s doesn't have OF-DPA world", name); + error_setg(errp, "rocker %s doesn't have OF-DPA world", name); return NULL; } @@ -2597,15 +2595,13 @@ RockerOfDpaGroupList *qmp_query_rocker_of_dpa_groups(const char *name, r = rocker_find(name); if (!r) { - error_set(errp, ERROR_CLASS_GENERIC_ERROR, - "rocker %s not found", name); + error_setg(errp, "rocker %s not found", name); return NULL; } w = rocker_get_world(r, ROCKER_WORLD_TYPE_OF_DPA); if (!w) { - error_set(errp, ERROR_CLASS_GENERIC_ERROR, - "rocker %s doesn't have OF-DPA world", name); + error_setg(errp, "rocker %s doesn't have OF-DPA world", name); return NULL; } diff --git a/qom/object.c b/qom/object.c index fc6e161..c0decb6 100644 --- a/qom/object.c +++ b/qom/object.c @@ -1330,8 +1330,8 @@ static Object *object_resolve_link(Object *obj, const char *name, target = object_resolve_path_type(path, target_type, &ambiguous); if (ambiguous) { - error_set(errp, ERROR_CLASS_GENERIC_ERROR, - "Path '%s' does not uniquely identify an object", path); + error_setg(errp, "Path '%s' does not uniquely identify an object", + path); } else if (!target) { target = object_resolve_path(path, &ambiguous); if (target || ambiguous) {
A few uses of error_set(ERROR_CLASS_GENERIC_ERROR) have snuck in since c6bd8c706. Nuke them. Signed-off-by: Eric Blake <eblake@redhat.com> --- v11: new patch --- block.c | 3 +-- docs/writing-qmp-commands.txt | 20 +++++++++----------- hw/i386/pc.c | 2 +- hw/net/rocker/rocker.c | 6 ++---- hw/net/rocker/rocker_of_dpa.c | 12 ++++-------- qom/object.c | 4 ++-- 6 files changed, 19 insertions(+), 28 deletions(-)