Re: [PATCH 1/3] mkfs/proto.c: avoid to use NULL pointer

Eric Sandeen
2022-06-06T14:29:39Z

On 6/6/22 7:33 AM, zhanchengbin wrote:
> Change malloc to xmalloc.

The commit log and cover letter say nothing about this, but apparently
"xmalloc" is often defined locally to abort() on a memory failure, so I
guess you are trying to make (some of?) xfsprogs do that.

First, this doesn't seem to build....

Building mkfs
[CC] proto.o
proto.c: In function ‘setup_proto’:
proto.c:73:15: warning: implicit declaration of function ‘xmalloc’; did you mean ‘malloc’? [-Wimplicit-function-declaration]
73 | buf = xmalloc(size + 1);
| ^~~~~~~
| malloc
proto.c:73:13: warning: assignment to ‘char *’ from ‘int’ makes pointer from integer without a cast [-Wint-conversion]
73 | buf = xmalloc(size + 1);
| ^
proto.c: In function ‘newregfile’:
proto.c:306:21: warning: assignment to ‘char *’ from ‘int’ makes pointer from integer without a cast [-Wint-conversion]
306 | buf = xmalloc(size);
| ^
[LD] mkfs.xfs
/usr/bin/ld: proto.o: in function `setup_proto':
/src/git/xfsprogs-dev/mkfs/proto.c:73: undefined reference to `xmalloc'
/usr/bin/ld: proto.o: in function `newregfile':
/src/git/xfsprogs-dev/mkfs/proto.c:306: undefined reference to `xmalloc'
collect2: error: ld returned 1 exit status
gmake[2]: *** [../include/buildrules:65: mkfs.xfs] Error 1
gmake[1]: *** [include/buildrules:36: mkfs] Error 2
make: *** [Makefile:92: default] Error 2

Second, we have calls to malloc all over the codebase, including around
100 outside of the internal libraries in the various userspace utilities.
Why change only mkfs/db/repair?

Third, what problem are you solving? Granted, we should be checking every
malloc call, and it seems that we don't. But have you ever seen these
failures in practice? Your commit log should at least explain why this
makes the code better (and, I guess, where xmalloc is supposed to come
from...)

-Eric

> Signed-off-by: zhanchengbin <zhanchengbin1@huawei.com>
> ---
>  mkfs/proto.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mkfs/proto.c b/mkfs/proto.c
> index 127d87dd..f3b8710c 100644
> --- a/mkfs/proto.c
> +++ b/mkfs/proto.c
> @@ -70,7 +70,7 @@ setup_proto(
>          goto out_fail;
>      }
>
> -    buf = malloc(size + 1);
> +    buf = xmalloc(size + 1);
>      if (read(fd, buf, size) < size) {
>          fprintf(stderr, _("%s: read failed on %s: %s\n"),
>              progname, fname, strerror(errno));
> @@ -303,7 +303,7 @@ newregfile(
>          exit(1);
>      }
>      if ((*len = (int)size)) {
> -        buf = malloc(size);
> +        buf = xmalloc(size);
>          if (read(fd, buf, size) < size) {
>              fprintf(stderr, _("%s: read failed on %s: %s\n"),
>                  progname, fname, strerror(errno));