[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:

[EMAIL PROTECTED] 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


_______________________________________________
Pvfs2-developers mailing list
Pvfs2-developers@beowulf-underground.org
http://www.beowulf-underground.org/mailman/listinfo/pvfs2-developers