[Ovirt-devel] [RFC]: Clean up ovirt-identify-node.c

[Jim Meyering]

Chris Lalancette <clalance at redhat.com> wrote:
> All,
>      Attached is a patch that cleans up ovirt-identify-node.c.  I haven't yet
> tested it (beyond compile-testing), but I just wanted to show people what I was
> doing with this.  It's a large patch, but in essence, it:
>
> 1)  Moves all variable declarations to the top of functions, for easier reading
> 2)  Converts all sprintf() to snprintf() for safety
> 3)  Removes a bunch of unnecessary buffers
> 4)  Fixes a couple of fprintf() compile warnings
> 5)  Makes saferead() and safewrite() actually safe
...
> @@ -300,7 +291,7 @@ ssize_t safewrite(int fd, const void *buf, size_t count)
>  {
>          size_t nwritten = 0;
>          while (count > 0) {
> -                ssize_t r = write(fd, buf, count);
> +                ssize_t r = write(fd, buf+nwritten, count);
>
>                  if (r < 0 && errno == EINTR)
>                          continue;

Hi Chris,

I haven't looked closely yet at the new saferead function,
but if you could explain how the old version is unsafe, that
might make it easier to see why you changed it.

The above change to safewrite introduces a bug, since not only is "buf"
a void* pointer (with which doing arithmetic is not portable), but "buf"
is already incremented just below:

                buf = (const char *)buf + r;

    ssize_t safewrite(int fd, const void *buf, size_t count)
    {
            size_t nwritten = 0;
            while (count > 0) {
                    ssize_t r = write(fd, buf+nwritten, count);

                    if (r < 0 && errno == EINTR)
                            continue;
                    if (r < 0)
                            return r;
                    if (r == 0)
                            return nwritten;
                    buf = (const char *)buf + r;
                    count -= r;
                    nwritten += r;
            }
            return nwritten;
    }

So, a short write of 1 byte would be followed by an attempt
to write buf[2], thus skipping buf[1].

I see your patch has been merged with another and checked in on "next".
I'll undo that part of the change with this patch: