[Pvfs2-developers] Re: [Pvfs2-cvs] commit by slang in pvfs2/src/common/misc: fsck-utils.c fsck-utils.h

[Sam Lang]

Hi Pete,

Thanks for the comments.  Everything seems reasonable to me.  I would  
probably just remove the EWARNING altogether and just return 0, since  
we gossip_err anyway.  The special characters...would it be easier to  
check for valid characters instead of invalid ones?

-sam

On Dec 6, 2006, at 11:48 AM, Pete Wyckoff wrote:

> cvs at parl.clemson.edu wrote on Wed, 06 Dec 2006 12:22 -0500:
>> Update of /projects/cvsroot/pvfs2/src/common/misc
>> In directory parlweb1:/tmp/cvs-serv15660/src/common/misc
>>
>> Added Files:
>> 	fsck-utils.c fsck-utils.h
>> Log Message:
>> missed adding these in recent commit of pvfs2-validate code.
> [..]
>> +/**
>> + * Performs validity checking for the PVFS_TYPE_DIRECTORY  
>> directory entries
>> + * Checks the following:
>> + * - invalid characters in entry names
>> + * - entry_names <= PVFS2_SEGMENT_MAX
>> + * - warnings for characters that tend to confuse shells
>> + * .
>> + * \retval 0 on success
>> + * \retval -PVFS_EWARNING for non critical warnings
>> + * \retval -PVFS_error on failure
>> + */
>> +int PVFS_fsck_validate_dir_ent(
>> +    const struct PINT_fsck_options *fsck_options, /**< generic  
>> fsck options */
>> +    const char *filename)              /**< Filename associated  
>> with handle */
>> +{
>> +    int ret = 0;
>> +
>> +    if (strlen(filename) > PVFS_SEGMENT_MAX)
>> +    {
>> +        gossip_err(
>> +                "Error: directory entry [%s] length is >  
>> PVFS_SEGMENT_MAX [%d]\n",
>> +                filename, PVFS_SEGMENT_MAX);
>> +        set_return_code(&ret, -PVFS_EINVAL);
>> +    }
>> +
>> +    if (strspn(filename, "/") > 0)
>> +    {
>> +        gossip_err("Error: directory entry [%s] contains  
>> invalid / character \n",
>> +                filename);
>> +        set_return_code(&ret, -PVFS_EINVAL);
>> +    }
>> +
>> +    if (strspn(filename, "\n") > 0)
>> +    {
>> +        gossip_err(
>> +                "WARNING: directory entry [%s] contains invalid  
>> new line character\n",
>> +                filename);
>> +        set_return_code(&ret, -PVFS_EWARNING);
>> +    }
>> +
>> +    if (strspn(filename, "\r") > 0)
>> +    {
>> +        gossip_err(
>> +                "WARNING: directory entry [%s] contains invalid  
>> carriage return character\n",
>> +                filename);
>> +        set_return_code(&ret, -PVFS_EWARNING);
>> +    }
>> +
>> +    if (strspn(filename, "|") > 0)
>> +    {
>> +        gossip_err(
>> +                "WARNING: directory entry [%s] contains invalid  
>> pipe character\n",
>> +                filename);
>> +        set_return_code(&ret, -PVFS_EWARNING);
>> +    }
>> +
>> +    if (strspn(filename, "*") > 0)
>> +    {
>> +        gossip_err(
>> +                "WARNING: directory entry [%s] contains invalid #  
>> character\n",
>> +                filename);
>> +        set_return_code(&ret, -PVFS_EWARNING);
>> +    }
>> +
>> +    if (strspn(filename, "?") > 0)
>> +    {
>> +        gossip_err(
>> +                "WARNING: directory entry [%s] contains invalid ?  
>> character\n",
>> +                filename);
>> +        set_return_code(&ret, -PVFS_EWARNING);
>> +    }
>> +
>> +    return ret;
>> +}
>
> I'm not too happy about this part.  Three reasons:
>
> 1.  -PVFS_EWARNING:  An error that is actually a warning?  There's
>     lots of spots in this code that look like:
>
> 	err = PVFS_do_something(..)
> 	if (err < 0 && err != -PVFS_EWARNING)
> 	    die()
>
>     It seems icky to have to have every caller filter out the errors
>     that aren't _real_ errors.
>
> 2.  There's nothing illegal about any of the characters above
>     except for '/' (and '\0').  Names "." and ".." have special
>     meaning but you handle that separately elsewhere.
>
> 3.  You're rather selective about the "bad practice" characters.
>     If *?|\r\n are bad, why not \010 or \177 or ! or # or ' or any
>     of the other characters that need a shell escape?  Seems rather
>     arbitrary.
>
> Can we get rid of these warnings and the infrastructure for
> handling the not-quite-an-error?
>
> 		-- Pete
>