Re: e2fsprogs patch for review (and other stuff)

[Guillem Jover <guillem@debian.org>]

Hi!

On Tue, 2011-10-04 at 23:30:14 +0200, Samuel Thibault wrote:
> Svante Signell, le Tue 04 Oct 2011 17:53:49 +0200, a écrit :
> > --- e2fsprogs-1.42~WIP-2011-10-01/lib/quota/mkquota.c	2011-09-18 20:06:52.000000000 +0200
> > +++ e2fsprogs-1.42~WIP-2011-10-01.modified//lib/quota/mkquota.c	2011-10-04 15:25:26.000000000 +0200
> > @@ -51,9 +51,13 @@
> >  	char tmp[1024];
> >  	qid_t id = (type == USRQUOTA) ? getuid() : getgid();
> >  
> > +#ifndef __GNU__
> >  	if (!quotactl(QCMD(Q_V2_GETQUOTA, type), fs->device_name, id, tmp))
> >  		return 1;
> >  	return 0;
> > +#else
> 
> Maybe rather use #ifdef Q_V2_GETQUOA?

Right, I've noticed you tend to do this, but I think it's in general
preferable to check for the positive condition: where is this
supported, rather than the opposite.

The function scope variables will not be needed either, so they can be
ifdefed out too, or they might generate warnings.

> > @@ -55,7 +56,11 @@
> >  const char *get_qf_name(int type, int fmt, char *buf)
> >  {
> >  	BUG_ON(!buf);
> > -	snprintf(buf, PATH_MAX, "%s.%s",
> > +	int len;
> > +	len = strlen(basenames[fmt]) + 1 + strlen(extensions[type]) + 1;
> > +	if( (buf = malloc(len)) == NULL)
> > +		 assert(buf);

Hmm, assert()ing on memory allocation error is pretty harsh and
non-descriptive, and it is customarily used on programming error,
not on external/system errors.

A small style nit, you might want to declare variables first, blank
line then code.

> Take care of such situation: assigning buf = malloc(len) will *not*
> change the value of the pointer in the caller, that is
> 
> > @@ -65,7 +70,7 @@
> >  			char *path_buf, size_t path_buf_size)
> >  {
> >  	struct stat	qf_stat;
> > -	char qf_name[PATH_MAX] = {0};
> > +	char *qf_name = NULL;
> 
> This one. You have to pass the address of the pointer to permit
> get_qf_name to change it. You'll also have to take care about
> allocations/frees to not leak.

Given that get_qf_name() is returning buf, just remove it from the
arguments, and assign qf_name to it from the call site.

regards,
guillem