Hi Piyush,
nfs4_srv_attr.c
---------------
1) val is ulong_t. You cannot compare it with -1:
1554 ulong_t val;
1582 if (val == -1) {
1603 if (val == -1) {
2)
1583 na->maxfilesize = MAXOFF32_T;
1604 maxfilesize = MAXOFF32_T;
Why MAXOFF32_T and not MAXOFFSET_T or MAXOFF_T or ...? Could you elaborate,
please? When _PC_FILESIZEBITS is supported you could return up to UINT64_MAX,
which is a _bit_ bigger than MAXOFF32_T.
3) Why there is comment in the NFS4ATTR_GETIT case only? I am missing some
comment in the NFS4ATTR_VERIT case.
4) When _PC_FILESIZEBITS is not supported VOP_PATHCONF() will fail and return
EINVAL (according to the comments section in the CR), so the codepath will
break at line 1574 and your fix won't be used to salvage the situation:
1571 error = VOP_PATHCONF(sarg->cs->vp, _PC_FILESIZEBITS, &val,
1572 sarg->cs->cr, NULL);
1573 if (error)
1574 break;
1575
1576 /*
1577 * If the underlying file system does not support
1578 * _PC_FILESIZEBITS, return a reasonable default. Note that
1579 * error code on VOP_PATHCONF will be 0, even if the
underlying
1580 * file system does not support _PC_FILESIZEBITS.
1581 */
1582 if (val == -1) {
1583 na->maxfilesize = MAXOFF32_T;
1584 } else {
1585 if (val >= (sizeof (uint64_t) * 8))
1586 na->maxfilesize = UINT64_MAX;
1587 else
1588 na->maxfilesize = ((1LL << val) - 1);
1589 }
nfs4_srv_readdir.c
------------------
Same (or very similar) questions as above applies here too.
Regards.
On Mon, Jun 15, 2009 at 05:03:26PM -0500, Piyush Shivam wrote:
> Hi All,
>
> I would like to request a code review for:
>
> 6224897 NFSV4 server and PCFS filesystem don't like to discuss
> _PC_FILESIZEBITS
>
> The CR is at:
> http://bugs.opensolaris.org/bugdatabase/view_bug.do?bug_id=6224897
>
> The webrev is at:
> http://cr.opensolaris.org/~pshivam/6224897_PC_FILESIZEBITS/
>
>
> Background:
>
> The NFSv4 server wants to determine the maximum file size of the
> underlying file system, e.g., at the client mount time. The server uses
> VOP_PATHCONF(_PC_FILESIZEBITS) call to do so. The current server
> implementation does not handle the case where the underlying file system
> does not support _PC_FILESIZEBITS pathconf query.
>
> The problem was first reported against PCFS. However, the problem no
> longer occurs with PCFS, since it supports _PC_FILESIZEBITS since
> snv_15. Nevertheless, NFSv4 needs to be more robust to the missing
> support for maximum file size from the underlying file system, and
> return a reasonable default.
>
> The current implementation seems to be doing the right thing
> accidentally (as far as I can tell). In the following check, the value
> of the variable val is -1 when the _PC_FILESIZEBITS is not supported by
> the underlying file system. However, since val is an unsigned long, the
> "if" condition evaluates to true, and UINT64_MAX is the default value
> for maxfilesize.
>
> 1573 error = VOP_PATHCONF(sarg->cs->vp,
> _PC_FILESIZEBITS, &val,
> 1574 sarg->cs->cr, NULL);
> 1575 if (error)
> 1576 break;
> 1577 if (val >= (sizeof (uint64_t) * 8))
> 1578 na->maxfilesize = UINT64_MAX;
> 1579 else
> 1580 na->maxfilesize = ((1LL << val) - 1);
--
Marcel Telka
Solaris RPE
