From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55920) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aGULi-0006ew-0B for qemu-devel@nongnu.org; Tue, 05 Jan 2016 11:20:43 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aGULc-0007t7-WD for qemu-devel@nongnu.org; Tue, 05 Jan 2016 11:20:41 -0500 Received: from mail-wm0-x233.google.com ([2a00:1450:400c:c09::233]:33695) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aGULc-0007t3-M8 for qemu-devel@nongnu.org; Tue, 05 Jan 2016 11:20:36 -0500 Received: by mail-wm0-x233.google.com with SMTP id f206so29215011wmf.0 for ; Tue, 05 Jan 2016 08:20:36 -0800 (PST) References: <1450355367-14818-1-git-send-email-eric.auger@linaro.org> <1450355367-14818-5-git-send-email-eric.auger@linaro.org> From: Eric Auger Message-ID: <568BED48.50404@linaro.org> Date: Tue, 5 Jan 2016 17:20:24 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 4/6] device_tree: qemu_fdt_getprop converted to use the error API List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Baptiste Reynal , Thomas Huth , eric.auger@st.com, Patch Tracking , Peter Crosthwaite , QEMU Developers , Alex Williamson , Suravee Suthikulpanit , Paolo Bonzini , thomas.lendacky@amd.com, =?UTF-8?Q?Alex_Benn=c3=a9e?= , Christoffer Dall , David Gibson Hi Peter, On 12/18/2015 03:36 PM, Peter Maydell wrote: > On 17 December 2015 at 12:29, Eric Auger 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 >> >> --- >> >> 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 >> --- >> 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 >