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

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

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.

> > -	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 ...

> > -	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. 

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