[PATCH 11/13] nfsd4: fslocations data structures

[J. Bruce Fields]

On Mon, Sep 04, 2006 at 05:49:16PM +1000, Neil Brown wrote:
> On Friday September 1, bfields at fieldses.org wrote:
> > +
> > +	err = -ENOMEM;
> > +	fsloc->locations = kzalloc(fsloc->locations_count
> > +			* sizeof(struct nfsd4_fs_location), GFP_KERNEL);
> > +	if (!fsloc->locations)
> > +		goto out_free_all;
> 
> Nope. This will de-reference that NULL that kzalloc returned.
> You just want a 'return -ENOMEM' there.
> Fixed

Thanks.

> 
> > +	/* migrated */
> > +	err = get_int(mesg, &migrated);
> > +	if (err)
> > +		goto out_free_all;
> > +	fsloc->migrated = migrated;
> 
> Should 'migrated' be range checked at all?
> I would prefer to reject any unexpected value.
> PleaseFix.

It's a boolean, so we only ever check whether it's nonzero or not.
But OK.

> > -static void exp_flags(struct seq_file *m, int flag, int fsid, uid_t anonu, uid_t anong)
> > +static void exp_flags(struct seq_file *m, int flag, int fsid,
> > +		uid_t anonu, uid_t anong, struct nfsd4_fs_locations *fsloc)
> >  {
> >  	int first = 0;
> >  	struct flags *flg;
> > @@ -1171,6 +1264,16 @@ static void exp_flags(struct seq_file *m
> >  		seq_printf(m, "%sanonuid=%d", first++?",":"", anonu);
> >  	if (anong != (gid_t)-2 && anong != (0x10000-2))
> >  		seq_printf(m, "%sanongid=%d", first++?",":"", anong);
> > +	if (fsloc && fsloc->locations_count > 0) {
> > +		char *loctype = (fsloc->migrated) ? "refer" : "replicas";
> > +		int i;
> > +
> > +		seq_printf(m, "%s%s=", first++?",":"", loctype);
> > +		seq_printf(m, "%s@%s", fsloc->locations[0].path, fsloc->locations[0].hosts);
> > +		for (i = 1; i < fsloc->locations_count; i++) {
> > +			seq_printf(m, ",%s@%s", fsloc->locations[i].path, fsloc->locations[i].hosts);
> > +		}
> > +	}
> 
> You need to seq_escape the 'path' and 'hosts' fields.
> And you have a comma separated list of path at host inside a comma
> separated list of flags.  That's not good.
> PleaseFix.

OK.--b.