On Thu, 10 Apr 2014 08:24:20 -0600
Eric Blake <eblake@xxxxxxxxxx> wrote:
> On 04/10/2014 08:04 AM, Natanael Copa wrote:
> > We must always reset errno to 0 even if we do 'continue'.
> >
> > This fixes runtime with musl libc which will set errno on sscanf.
> >
> > Signed-off-by: Natanael Copa <ncopa@xxxxxxxxxxxxxxx>
> > ---
> > src/nodeinfo.c | 15 +++------------
> > 1 file changed, 3 insertions(+), 12 deletions(-)
> >
> > diff --git a/src/nodeinfo.c b/src/nodeinfo.c
> > index 53ba716..8d3214e 100644
> > --- a/src/nodeinfo.c
> > +++ b/src/nodeinfo.c
> > @@ -452,8 +452,7 @@ virNodeParseNode(const char *node,
> >
> > /* enumerate sockets in the node */
> > CPU_ZERO(&sock_map);
> > - errno = 0;
> > - while ((cpudirent = readdir(cpudir))) {
> > + for (errno = 0; (cpudirent = readdir(cpudir)); errno = 0) {
> > if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1)
> > continue;
>
> Good catch. However, the code is still buggy even after your fix. We
> are missing:
>
> if (errno)
> break;
>
> before attempting the sscanf.
Why? if readdir fails it should return NULL and the for() loop
condition should break the loop. If readdir does not return NULL it
didn't fail and errno value is undefined.
I suppose we could use helper function to make it more readable:
static struct dirent *virReaddir(DIR *dirp)
{
errno = 0;
return readdir(dirp);
}
...
while ((cpudirent = virReaddir(cpudir))
> Furthermore, sscanf() is undefined on
> overflow; while the cpudir is unlikely to be giving us integers that
> overflow, it would be nicer to not use sscanf in the first place. It's
> more than just the improper use of readdir that needs fixing here.
>
> I'm debating whether to push this now and fix the rest as a followup, or
> whether to do a v2 that fixes it all at once.
I'd say fix the current bug first and do the clean up in separate commit.
-nc
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list