| Message ID | 44ef0950-791d-e17e-1fe8-f58d3148603f@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xfsprogs: avoid to use NULL pointer | expand |
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));
On Mon, Jun 06, 2022 at 09:29:27AM -0500, Eric Sandeen wrote: > 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? xmalloc is local to xfs_db (db/malloc.c), it's not part of libxfs. It's local to xfs_db because it is the tool that always runs out of memory on large filesystems. It's just a wrapper that prints "out of memory" and then exits immediately with no chance to recover. So, essentially, it's no different to immediately SEGV on a NULL pointer and dying. > Third, what problem are you solving? Granted, we should be checking every > malloc call, and it seems that we don't. In general, that's because we can't do anything useful to free memory when we fail memory allocation - we generally exit immediately on memory allocation failure, so checking for failure is almost redundant.... Cheers, Dave.
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));
Change malloc to xmalloc. Signed-off-by: zhanchengbin <zhanchengbin1@huawei.com> --- mkfs/proto.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)