Re: [PATCH v2] erofs-utils: complete missing memory handling

[Gao Xiang <gaoxiang25@huawei.com>]

On Thu, Nov 14, 2019 at 08:24:31AM +0800, Gao Xiang via Linux-erofs wrote:
> Hi Guifu,
> 
> On Thu, Nov 14, 2019 at 01:03:35AM +0800, Li Guifu wrote:
> > From: Li Guifu <bluce.liguifu@huawei.com>
> > 
> > memory allocation failure should be handled
> > properly in principle.
> > 
> > Signed-off-by: Li Guifu <bluce.liguifu@huawei.com>
> > [ Gao Xiang: due to Huawei outgoing email limitation,
> >   I have to help Guifu send out his patches at work. ]
> > Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
> > Signed-off-by: Li Guifu <blucerlee@gmail.com>
> > ---
> 
> As a common practice, It's perferred to leave some useful
> comments at this about what you modified compared wtih
> the last version.
> 
> >  lib/inode.c | 21 ++++++++++++++++++---
> >  1 file changed, 18 insertions(+), 3 deletions(-)
> > 
> > diff --git a/lib/inode.c b/lib/inode.c
> > index 86c465e..b6c2b13 100644
> > --- a/lib/inode.c
> > +++ b/lib/inode.c
> > @@ -264,6 +264,8 @@ int erofs_write_dir_file(struct erofs_inode *dir)
> >  	if (used) {
> >  		/* fill tail-end dir block */
> >  		dir->idata = malloc(used);
> > +		if (!dir->idata)
> > +			return -ENOMEM;
> >  		DBG_BUGON(used != dir->idata_size);
> >  		fill_dirblock(dir->idata, dir->idata_size, q, head, d);
> >  	}
> > @@ -286,6 +288,8 @@ int erofs_write_file_from_buffer(struct erofs_inode *inode, char *buf)
> >  	inode->idata_size = inode->i_size % EROFS_BLKSIZ;
> >  	if (inode->idata_size) {
> >  		inode->idata = malloc(inode->idata_size);
> > +		if (!inode->idata)
> > +			return -ENOMEM;
> >  		memcpy(inode->idata, buf + blknr_to_addr(nblocks),
> >  		       inode->idata_size);
> >  	}
> > @@ -347,9 +351,14 @@ int erofs_write_file(struct erofs_inode *inode)
> >  	inode->idata_size = inode->i_size % EROFS_BLKSIZ;
> >  	if (inode->idata_size) {
> >  		inode->idata = malloc(inode->idata_size);
> > -
> > +		if (!inode->idata) {
> > +			errno = ENOMEM;
> > +			goto fail;
> > +		}

When I revisited this patch, I noticed it's some weird to operate `errno' here.

The same sequence "close(fd); return -ENOMEM;" is indeed some unclean, but
I think you could make a separate cleanup patch then.

I will resend PATCH v3 since you aren't able to post public mail at work
and apply it to experimental branch.

Thanks,
Gao Xiang

> >  		ret = read(fd, inode->idata, inode->idata_size);
> >  		if (ret < inode->idata_size) {
> > +			free(inode->idata);
> > +			inode->idata = NULL;
> 
> Anyway, it seems the diffstat is this line.
> I think it' better than v1 so let's use this version.
> 
> Thanks,
> Gao Xiang
>