| Message ID | 20191205174635.18758-7-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | error: prepare for auto propagated local_err | expand |
On 12/5/19 11:46 AM, Vladimir Sementsov-Ogievskiy wrote: > Make qbus_list_bus and qbus_list_dev hint append helpers well formed: > switch errp paramter to Error *const * type, as it has uncommon > behavior: not change the pointer to return error, but operate on > already existent error object. > Rename functions to be error_append_*_hint. qbus_error_append_*_hint > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > qdev-monitor.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > -static void qbus_list_dev(BusState *bus, Error **errp) > +static void qbus_error_append_dev_list_hint(BusState *bus, > + Error *const *errp) Or, since you copy-and-paste this commit message in v9, you could use *_error_append_*_hint between the commits for less one-off fixing per message.
On 12/5/19 12:17 PM, Eric Blake wrote: > On 12/5/19 11:46 AM, Vladimir Sementsov-Ogievskiy wrote: >> Make qbus_list_bus and qbus_list_dev hint append helpers well formed: >> switch errp paramter to Error *const * type, as it has uncommon >> behavior: not change the pointer to return error, but operate on >> already existent error object. >> Rename functions to be error_append_*_hint. > > qbus_error_append_*_hint > >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> qdev-monitor.c | 12 +++++++----- >> 1 file changed, 7 insertions(+), 5 deletions(-) >> > >> -static void qbus_list_dev(BusState *bus, Error **errp) >> +static void qbus_error_append_dev_list_hint(BusState *bus, >> + Error *const *errp) > > Or, since you copy-and-paste this commit message in v9, you could use Sorry, I meant it was copied in v8 7/21. > *_error_append_*_hint between the commits for less one-off fixing per > message. >
On 12/5/19 6:46 PM, Vladimir Sementsov-Ogievskiy wrote: > Make qbus_list_bus and qbus_list_dev hint append helpers well formed: > switch errp paramter to Error *const * type, as it has uncommon "parameter" > behavior: not change the pointer to return error, but operate on > already existent error object. > Rename functions to be error_append_*_hint. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: > Make qbus_list_bus and qbus_list_dev hint append helpers well formed: > switch errp paramter to Error *const * type, as it has uncommon > behavior: not change the pointer to return error, but operate on > already existent error object. > Rename functions to be error_append_*_hint. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> "well form error hint helpers" tells me nothing. PATCH 03 does the same (less function renames), and is titled "make Error **errp const where it is appropriate". I like that better. No big deal, and certainly not worth a respin by itself. I'm happy to touch up commit messages in my tree.
Markus Armbruster <armbru@redhat.com> writes: > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: > >> Make qbus_list_bus and qbus_list_dev hint append helpers well formed: >> switch errp paramter to Error *const * type, as it has uncommon >> behavior: not change the pointer to return error, but operate on >> already existent error object. >> Rename functions to be error_append_*_hint. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > > "well form error hint helpers" tells me nothing. PATCH 03 does the same > (less function renames), and is titled "make Error **errp const where it > is appropriate". I like that better. No big deal, and certainly not > worth a respin by itself. I'm happy to touch up commit messages in my > tree. Let's adapt PATCH 03's commit message, like this: qdev-monitor: make Error **errp const where it is appropriate Mostly, Error ** is for returning error from the function, so the callee sets it. However qbus_list_bus and qbus_list_dev get already filled errp parameter. They don't change the pointer itself, only change the internal state of referenced Error object. So we can make it Error *const * errp, to stress the behavior. It will also help coccinelle script (in future) to distinguish such cases from common errp usage. While there, rename the functions to qbus_error_append_bus_list_hint(), qbus_error_append_dev_list_hint(). With something like that: Reviewed-by: Markus Armbruster <armbru@redhat.com>
diff --git a/qdev-monitor.c b/qdev-monitor.c index 29ed73e56a..3465a1e2d0 100644 --- a/qdev-monitor.c +++ b/qdev-monitor.c @@ -328,7 +328,8 @@ static Object *qdev_get_peripheral_anon(void) return dev; } -static void qbus_list_bus(DeviceState *dev, Error **errp) +static void qbus_error_append_bus_list_hint(DeviceState *dev, + Error *const *errp) { BusState *child; const char *sep = " "; @@ -342,7 +343,8 @@ static void qbus_list_bus(DeviceState *dev, Error **errp) error_append_hint(errp, "\n"); } -static void qbus_list_dev(BusState *bus, Error **errp) +static void qbus_error_append_dev_list_hint(BusState *bus, + Error *const *errp) { BusChild *kid; const char *sep = " "; @@ -500,7 +502,7 @@ static BusState *qbus_find(const char *path, Error **errp) if (!dev) { error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, "Device '%s' not found", elem); - qbus_list_dev(bus, errp); + qbus_error_append_dev_list_hint(bus, errp); return NULL; } @@ -518,7 +520,7 @@ static BusState *qbus_find(const char *path, Error **errp) if (dev->num_child_bus) { error_setg(errp, "Device '%s' has multiple child buses", elem); - qbus_list_bus(dev, errp); + qbus_error_append_bus_list_hint(dev, errp); } else { error_setg(errp, "Device '%s' has no child bus", elem); } @@ -534,7 +536,7 @@ static BusState *qbus_find(const char *path, Error **errp) bus = qbus_find_bus(dev, elem); if (!bus) { error_setg(errp, "Bus '%s' not found", elem); - qbus_list_bus(dev, errp); + qbus_error_append_bus_list_hint(dev, errp); return NULL; } }
Make qbus_list_bus and qbus_list_dev hint append helpers well formed: switch errp paramter to Error *const * type, as it has uncommon behavior: not change the pointer to return error, but operate on already existent error object. Rename functions to be error_append_*_hint. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- qdev-monitor.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)