Re: [PATCH 8/9] nfs: switch to nfs3

[Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>]

On 09:52 Fri 07 Feb     , Uwe Kleine-K??nig wrote:
> Hello Jean-Christophe,
> 
> On Fri, Feb 07, 2014 at 07:48:57AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 17:40 Thu 06 Feb     , Uwe Kleine-K??nig wrote:
> > > +#define ntohll(val)	__be64_to_cpu(val)
> > > +#define htonll(val)	__cpu_to_be64(val)
> > 
> > use the cpu_to and to_cpu directly
> > as if we have the standard define in any header later this will cause issues
> htonll isn't that standard. I doesn't exist neither in Linux userspace
> nor in Linux kernel space. I would prefer to put the macro into a global
> place already now. Or still better, define {ntoh,hton}{16,32,64} which
> have a more intuitive naming scheme.
yeah agreed
> 
> > > -	printk("NFS: returned filename too long: %u\n", count);
> > > +	printf("%s: returned a too long filename: %u\n", __func__, count);
> > can we use dev_xx for message
> I will check where I can get my hands on a good struct device_d*.
> 
> > > +static uint32_t *nfs_add_fh3(uint32_t *p, unsigned fh_len, const char *fh)
> > > +{
> > > +	*p++ = htonl(fh_len);
> > > +
> > > +	/* zero padding */
> > > +	if (fh_len & 3)
> > > +		p[fh_len / 4] = 0;
> > > +
> > > +	memcpy(p, fh, fh_len);
> > > +	p += DIV_ROUND_UP(fh_len, 4);
> > > +	return p;
> > > +}
> > > +
> > > +static uint32_t *nfs_add_filename(uint32_t *p,
> > > +		uint32_t filename_len, const char *filename)
> > > +{
> > > +	*p++ = htonl(filename_len);
> > > +
> > > +	/* zero padding */
> > > +	if (filename_len & 3)
> > > +		p[filename_len / 4] = 0;
> > > +
> > > +	memcpy(p, filename, filename_len);
> > > +	p += DIV_ROUND_UP(filename_len, 4);
> > > +	return p;
> > > +}
> > > +
> > what is the difference with the function upper?
> The function and parameter names and the type of the 2nd argument.
;)
> (OK, I guess that wasn't the answer you wanted to read. Yes, you're
> right, they could use the same function. Something like nfs_add_string.
> (Technically fh3 isn't a string<> but an opaque<NFS3_FHSIZE>, but that
> doesn't really matter here. I will think a bit about proper naming.))
> 
> > > +	npriv->rootfh_len = ntohl(net_read_uint32(p++));
> > > +	if (npriv->rootfh_len > NFS3_FHSIZE) {
> > > +		printf("%s: file handle too big: %lu\n", __func__,
> > > +				(unsigned long)npriv->rootfh_len);
> > > +		return -EIO;
> > really EIO?
> That's a protocol error and -EIO is what is returned in other places for
> protocol errors, too. Still if you have a better suggestion ...

-EPROTO no?
> 
> > > -	ret = rpc_lookup_req(npriv, PROG_NFS, 2);
> > > +	ret = rpc_lookup_req(npriv, PROG_NFS, 3);
> > 
> > so we loose nfs2?
> Right. Do you consider it a loss? I don't think it worth to implement
> both side by side. 

I see this as a compatibility issue
> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox