| Message ID | 1450355367-14818-5-git-send-email-eric.auger@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 17 December 2015 at 12:29, Eric Auger <eric.auger@linaro.org> wrote: > Current qemu_fdt_getprop exits if the property is not found. It is > sometimes needed to read an optional property, in which case we do > not wish to exit but simply returns a null value. > > This patch converts qemu_fdt_getprop to accept an Error **, and existing > users are converted to pass &error_fatal. This preserves the existing > behaviour. Then to use the API with your optional semantic a null > parameter can be conveyed. > > Signed-off-by: Eric Auger <eric.auger@linaro.org> > > --- > > RFC -> v1: > - get rid of qemu_fdt_getprop_optional and implement Peter's suggestion > that consists in using the error API This doesn't seem to me like a great way for qemu_fdt_getprop to report "property not found", because there's no way for the caller to distinguish "property not found" from "function went wrong some other way" (since Errors just report human readable strings, and in any case you're not distinguishing -FDT_ERR_NOTFOUND from any of the other FDT errors). If we want to handle "ok if property doesn't exist" then we could either (a) make the function return NULL on doesn't-exist and error_report in the other error cases, with the existing single caller extending its error checking appropriately, or (b) have the caller that cares about property-may-not-exist call fdt_getprop() directly. > Signed-off-by: Eric Auger <eric.auger@linaro.org> > --- > device_tree.c | 11 ++++++----- > include/sysemu/device_tree.h | 3 ++- > 2 files changed, 8 insertions(+), 6 deletions(-) > > diff --git a/device_tree.c b/device_tree.c > index b5d7e0b..c3720c2 100644 > --- a/device_tree.c > +++ b/device_tree.c > @@ -331,18 +331,18 @@ int qemu_fdt_setprop_string(void *fdt, const char *node_path, > } > > const void *qemu_fdt_getprop(void *fdt, const char *node_path, > - const char *property, int *lenp) > + const char *property, int *lenp, Error **errp) > { > int len; > const void *r; > + > if (!lenp) { > lenp = &len; > } > r = fdt_getprop(fdt, findnode_nofail(fdt, node_path), property, lenp); > if (!r) { > - error_report("%s: Couldn't get %s/%s: %s", __func__, > - node_path, property, fdt_strerror(*lenp)); > - exit(1); > + error_setg(errp, "%s: Couldn't get %s/%s: %s", __func__, > + node_path, property, fdt_strerror(*lenp)); > } > return r; > } > @@ -351,7 +351,8 @@ uint32_t qemu_fdt_getprop_cell(void *fdt, const char *node_path, > const char *property) > { > int len; > - const uint32_t *p = qemu_fdt_getprop(fdt, node_path, property, &len); > + const uint32_t *p = qemu_fdt_getprop(fdt, node_path, property, &len, > + &error_fatal); > if (len != 4) { > error_report("%s: %s/%s not 4 bytes long (not a cell?)", > __func__, node_path, property); > diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h > index f9e6e6e..284fd3b 100644 > --- a/include/sysemu/device_tree.h > +++ b/include/sysemu/device_tree.h > @@ -33,7 +33,8 @@ int qemu_fdt_setprop_phandle(void *fdt, const char *node_path, > const char *property, > const char *target_node_path); > const void *qemu_fdt_getprop(void *fdt, const char *node_path, > - const char *property, int *lenp); > + const char *property, int *lenp, > + Error **errp); If we change the function it would be nice to add a brief doc comment while we're touching the prototype in the header. thanks -- PMM
Hi Peter, On 12/18/2015 03:36 PM, Peter Maydell wrote: > On 17 December 2015 at 12:29, Eric Auger <eric.auger@linaro.org> wrote: >> Current qemu_fdt_getprop exits if the property is not found. It is >> sometimes needed to read an optional property, in which case we do >> not wish to exit but simply returns a null value. >> >> This patch converts qemu_fdt_getprop to accept an Error **, and existing >> users are converted to pass &error_fatal. This preserves the existing >> behaviour. Then to use the API with your optional semantic a null >> parameter can be conveyed. >> >> Signed-off-by: Eric Auger <eric.auger@linaro.org> >> >> --- >> >> RFC -> v1: >> - get rid of qemu_fdt_getprop_optional and implement Peter's suggestion >> that consists in using the error API > > This doesn't seem to me like a great way for qemu_fdt_getprop to > report "property not found", because there's no way for the caller > to distinguish "property not found" from "function went wrong > some other way" (since Errors just report human readable strings, > and in any case you're not distinguishing -FDT_ERR_NOTFOUND > from any of the other FDT errors). Not sure I get what you mean here. In case fdt_getprop fails, as long as the caller provided a lenp != NULL, *lenp contains the error code so qemu_fdt_getprop's caller can discriminate a -FDT_ERR_NOTFOUND from any other errors. Do I miss something? > > If we want to handle "ok if property doesn't exist" then we > could either (a) make the function return NULL on doesn't-exist > and error_report in the other error cases, with the existing > single caller extending its error checking appropriately, or > (b) have the caller that cares about property-may-not-exist > call fdt_getprop() directly. > >> Signed-off-by: Eric Auger <eric.auger@linaro.org> >> --- >> device_tree.c | 11 ++++++----- >> include/sysemu/device_tree.h | 3 ++- >> 2 files changed, 8 insertions(+), 6 deletions(-) >> >> diff --git a/device_tree.c b/device_tree.c >> index b5d7e0b..c3720c2 100644 >> --- a/device_tree.c >> +++ b/device_tree.c >> @@ -331,18 +331,18 @@ int qemu_fdt_setprop_string(void *fdt, const char *node_path, >> } >> >> const void *qemu_fdt_getprop(void *fdt, const char *node_path, >> - const char *property, int *lenp) >> + const char *property, int *lenp, Error **errp) >> { >> int len; >> const void *r; >> + >> if (!lenp) { >> lenp = &len; >> } >> r = fdt_getprop(fdt, findnode_nofail(fdt, node_path), property, lenp); >> if (!r) { >> - error_report("%s: Couldn't get %s/%s: %s", __func__, >> - node_path, property, fdt_strerror(*lenp)); >> - exit(1); >> + error_setg(errp, "%s: Couldn't get %s/%s: %s", __func__, >> + node_path, property, fdt_strerror(*lenp)); >> } >> return r; >> } >> @@ -351,7 +351,8 @@ uint32_t qemu_fdt_getprop_cell(void *fdt, const char *node_path, >> const char *property) >> { >> int len; >> - const uint32_t *p = qemu_fdt_getprop(fdt, node_path, property, &len); >> + const uint32_t *p = qemu_fdt_getprop(fdt, node_path, property, &len, >> + &error_fatal); >> if (len != 4) { >> error_report("%s: %s/%s not 4 bytes long (not a cell?)", >> __func__, node_path, property); >> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h >> index f9e6e6e..284fd3b 100644 >> --- a/include/sysemu/device_tree.h >> +++ b/include/sysemu/device_tree.h >> @@ -33,7 +33,8 @@ int qemu_fdt_setprop_phandle(void *fdt, const char *node_path, >> const char *property, >> const char *target_node_path); >> const void *qemu_fdt_getprop(void *fdt, const char *node_path, >> - const char *property, int *lenp); >> + const char *property, int *lenp, >> + Error **errp); > > If we change the function it would be nice to add a brief > doc comment while we're touching the prototype in the header. sure Thanks Eric > > thanks > -- PMM >
On 5 January 2016 at 16:20, Eric Auger <eric.auger@linaro.org> wrote: > Hi Peter, > On 12/18/2015 03:36 PM, Peter Maydell wrote: >> On 17 December 2015 at 12:29, Eric Auger <eric.auger@linaro.org> wrote: >>> Current qemu_fdt_getprop exits if the property is not found. It is >>> sometimes needed to read an optional property, in which case we do >>> not wish to exit but simply returns a null value. >>> >>> This patch converts qemu_fdt_getprop to accept an Error **, and existing >>> users are converted to pass &error_fatal. This preserves the existing >>> behaviour. Then to use the API with your optional semantic a null >>> parameter can be conveyed. >>> >>> Signed-off-by: Eric Auger <eric.auger@linaro.org> >>> >>> --- >>> >>> RFC -> v1: >>> - get rid of qemu_fdt_getprop_optional and implement Peter's suggestion >>> that consists in using the error API >> >> This doesn't seem to me like a great way for qemu_fdt_getprop to >> report "property not found", because there's no way for the caller >> to distinguish "property not found" from "function went wrong >> some other way" (since Errors just report human readable strings, >> and in any case you're not distinguishing -FDT_ERR_NOTFOUND >> from any of the other FDT errors). > Not sure I get what you mean here. In case fdt_getprop fails, as long as > the caller provided a lenp != NULL, *lenp contains the error code so > qemu_fdt_getprop's caller can discriminate a -FDT_ERR_NOTFOUND from any > other errors. Do I miss something? There's no documentation of this behaviour of qemu_fdt_getprop in either this commit message or in a doc comment in the header, so I didn't realise that was your intention. thanks -- PMM
diff --git a/device_tree.c b/device_tree.c index b5d7e0b..c3720c2 100644 --- a/device_tree.c +++ b/device_tree.c @@ -331,18 +331,18 @@ int qemu_fdt_setprop_string(void *fdt, const char *node_path, } const void *qemu_fdt_getprop(void *fdt, const char *node_path, - const char *property, int *lenp) + const char *property, int *lenp, Error **errp) { int len; const void *r; + if (!lenp) { lenp = &len; } r = fdt_getprop(fdt, findnode_nofail(fdt, node_path), property, lenp); if (!r) { - error_report("%s: Couldn't get %s/%s: %s", __func__, - node_path, property, fdt_strerror(*lenp)); - exit(1); + error_setg(errp, "%s: Couldn't get %s/%s: %s", __func__, + node_path, property, fdt_strerror(*lenp)); } return r; } @@ -351,7 +351,8 @@ uint32_t qemu_fdt_getprop_cell(void *fdt, const char *node_path, const char *property) { int len; - const uint32_t *p = qemu_fdt_getprop(fdt, node_path, property, &len); + const uint32_t *p = qemu_fdt_getprop(fdt, node_path, property, &len, + &error_fatal); if (len != 4) { error_report("%s: %s/%s not 4 bytes long (not a cell?)", __func__, node_path, property); diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h index f9e6e6e..284fd3b 100644 --- a/include/sysemu/device_tree.h +++ b/include/sysemu/device_tree.h @@ -33,7 +33,8 @@ int qemu_fdt_setprop_phandle(void *fdt, const char *node_path, const char *property, const char *target_node_path); const void *qemu_fdt_getprop(void *fdt, const char *node_path, - const char *property, int *lenp); + const char *property, int *lenp, + Error **errp); uint32_t qemu_fdt_getprop_cell(void *fdt, const char *node_path, const char *property); uint32_t qemu_fdt_get_phandle(void *fdt, const char *path);