On Wed, Jul 07, 2010 at 02:03:21PM -0400, David P. Quigley wrote:
> Comments inline
>
> On Wed, 2010-07-07 at 13:21 -0400, J. Bruce Fields wrote:
> > On Wed, Jul 07, 2010 at 10:31:26AM -0400, David P. Quigley wrote:
> > > This patch adds the ability to encode and decode file labels on the server for
> > > static __be32
> > > nfsd4_decode_fattr(struct nfsd4_compoundargs *argp, u32 *bmval,
> > > - struct iattr *iattr, struct nfs4_acl **acl)
> > > + struct iattr *iattr, struct nfs4_acl **acl,
> > > + struct nfs4_label **label)
> >
> > As we add more arguments, I wonder if at some point it becomes worth
> > creating something like
> >
> > struct nfsd4_attrs {
> > struct iattr iattr;
> > struct nfs4_acl *acl;
> > ...
> > }
> >
> > and passing that instead?
>
> I can definitely submit something like that as a stand alone patch and
> then add our nfs4_label stuff to that instead.
Not a big deal, but welcome if such a thing looks more generally useful
(say, if the same arguments are passed elsewhere).
> > > +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> > > + if (bmval[1] & FATTR4_WORD1_SECURITY_LABEL) {
> > > + uint32_t lfs;
> > > +
> > > + READ_BUF(4);
> > > + len += 4;
> > > + READ32(lfs);
> > > + READ_BUF(4);
> > > + len += 4;
> > > + READ32(dummy32);
> > > + READ_BUF(dummy32);
> > > + len += (XDR_QUADLEN(dummy32) << 2);
> > > + READMEM(buf, dummy32);
> > > +
> > > + if (dummy32 > NFS4_MAXLABELLEN)
> > > + return nfserr_resource;
> > > +
> > > + *label = kzalloc(sizeof(struct nfs4_label), GFP_KERNEL);
> >
> > Could we allocate this some toher way (it's small, right?) and avoid the
> > extra dynamic allocation here, just for simplicity's sake?
>
> How would you suggest going about doing that?
Maybe make op_label, cr_label, etc., a struct nfs4_label instead of a
struct nfs4_label *?
>
> >
> > > + if (*label == NULL) {
> > > + host_err = -ENOMEM;
> > > + goto out_nfserr;
> > > + }
> > > +
> > > + (*label)->label = kmalloc(dummy32 + 1, GFP_KERNEL);
> >
> > Might be nice to arrange NFS4_MAXLABELLEN to ensure this is never a
> > higher-order allocation.
>
> This should never be more than 4096. Under what circumstances would this
> become a higher-order allocation?
Can't dummy32+1 be 4097?
--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
