On Tue, 16 Jun 2009 08:11:34 +0200, Frank Batschulat (Home) <Frank.Batschulat
at sun.com> wrote:
> On Tue, 16 Jun 2009 07:31:35 +0200, Marcel Telka <Marcel.Telka at sun.com>
> wrote:
>
>> 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 }
>
> I disagree, you have to distinguish between the fact that _PC_FILESIZEBITS is
> not implemented and VOP_PATHCONF() not being implemented or returning an
> error.
>
> The error out you complain about is the latter fact, ie. VOP_PATHCONF()
> is not implemented by the underlaying file system at all and thus the default
> VOP is
> fs_nosys() ie. returning ENOSYS. bailing out here is the correct course of
> action.
>
> Or, the other part is if the underlaying file systems routine does implement
> VOP_PATHCONF() but does not provide _PC_FILESIZEBITS component and does return
> an error, like pcfs in the past with EINVAL. bailing out with an error is
> correct here as well.
>
> In general, the underlaying file system should for everything in fpathconf(2)
> it does not support
> default to the generic fs_pathconf() - I've fixed pcfs to adhere to this via
> 6656370 .
>
> finally fs_pathconf() will deal with the fact that the underlaying file system
> does not support _PC_FILESIZEBITS and will return -1 and _no_ error returned.
>
> 486 case _PC_FILESIZEBITS:
> 487
> 488 /*
> 489 * If ever we come here it means that underlying file
> system
> 490 * does not recognise the command and therefore this
> 491 * configurable limit cannot be determined. We return -1
> 492 * and don't change errno.
> 493 */
> 494
> 495 val = (ulong_t)-1; /* large file support */
> 496 break;
>
> which is where Piyush's fix comes into play finally.
>
> There are corner cases for certain non-fpathconf(2) calls where the default
> fs_pathconf()
> action does not make sense for the underlaying file system and it still may
> be forced
> to directly return an error here, like pcfs_pathconf() for _PC_CASE_BEHAVIOR,
> for which the fs_pathconf()
> default of _CASE_SENSITIVE is wrong (FAT by definition is case insensitive)
> but on the other hand pcfs does not implement the VFSFT_CASEINSENSITIVE
> feature
> and it is not part of the standard fpathconf(2) query table. fwiw, we have a
> bug open already
> to change pcfs here too, see 6621526.
>
> UFS on the other hand does not need to special case this as it is
> _CASE_SENSITIVE
> so the default action eventually taken in fs_pathconf() fallback from
> ufs_l_pathconf()
> does fit our needs.
I forgott to mention another reason for VOP_PATHCONF() legally returning an
error
and for which case we shall obey to the error being returned instead of
guessing.
per the standard, there are several pathconf(2) items that will legally cause
unspecified
results in some cases, according to the standard, _PC_FILESIZEBITS is one of
them.
http://www.opengroup.org/onlinepubs/009695399/functions/pathconf.html
Variable Value of name Requirements
{FILESIZEBITS} _PC_FILESIZEBITS 3,4
4. If path or fildes does not refer to a directory, it is unspecified whether
an implementation supports an association of the variable name with the
specified file.
so an implementation would be allowed to return EINVAL if VOP_PATHCONF() is
called
on a non-directory vnode in which case we should'nt guess and lie but bail out.
cheers
frankB
