On 01/26/2015 06:04 PM, Eric Blake wrote:
On 01/26/2015 02:56 PM, Laine Stump wrote:
> add_link_info() sets an error message when it fails to read
> /sys/class/net/$dev/operstate, but unfortunately frees the path string
> just prior to setting the error message, so the error string has
> "(null)" instead (thanks to glibc's asprintf noticing the null
> pointer).
If the file weren't Linux-only, I'd point out that passing NULL to other
libc's printf is likely to segfault - even less friendly :)
> Additionally, the macro used to create the log message,
> ERR_THROW_STRERROR() already turns errno into a string in the locally
> defined errbuf, but we aren't including it in the format string, so it
> doesn't get printed.
>
> This patch makes sure the path isn't freed until it is really no
> longer needed, and adds errbuf to the log message.
>
> This will hopefully help to find the root cause of
>
>
https://bugzilla.redhat.com/show_bug.cgi?id=1185850
> ---
> src/dutil_linux.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> @@ -1081,14 +1081,15 @@ static void add_link_info(struct netcf *ncf,
> xasprintf(&path, "/sys/class/net/%s/operstate", ifname);
> ERR_NOMEM(!path, ncf);
> state = read_file(path, &length);
> - FREE(path);
> - ERR_THROW_STRERROR(!state, ncf, EFILE, "Failed to read %s", path);
> + ERR_THROW_STRERROR(!state, ncf, EFILE, "Failed to read %s : %s",
> + path, strerror);
'strerror' is the address of a function - printing that as a string is
probably quite the wrong thing to do. I think you want 'errbuf' here.
Oops. Yeah, that's what my mind was thinking, but my fingers were in a
hurry. Proof once again that it's never good to push without a review.
> if ((nl = strchr(state, '\n')))
> *nl = 0;
> prop = xmlSetProp(link_node, BAD_CAST "state", BAD_CAST state);
> ERR_NOMEM(!prop, ncf);
>
> if (!strcmp(state, "up")) {
> + FREE(path);
> xasprintf(&path, "/sys/class/net/%s/speed", ifname);
> ERR_NOMEM(path == NULL, ncf);
> speed = read_file(path, &length);
> @@ -1100,7 +1101,8 @@ static void add_link_info(struct netcf *ncf,
> speed = strdup("0");
> ERR_NOMEM(!speed, ncf);
> }
> - ERR_THROW_STRERROR(!speed, ncf, EFILE, "Failed to read %s",
path);
> + ERR_THROW_STRERROR(!speed, ncf, EFILE, "Failed to read %s : %s",
> + path, strerror);
and again.
I assume ACK once I fix the bonehead finger fumble?