[linux-cifs-client] review 1, was Re: projected date for mount.cifs to support DFS junction points

[Christoph Hellwig]

Unfortunately I couldn't find an mbox archive of the cifs client list
anywhere, so I'll send you the review in reply to this mail, with
one reply per patch.

This is for the first patch:



+ *  fs/cifs/cifs_dfs_ref.c

Please don't mention file names in top of file comments, they serve no
use and get out of sync far too easily.  (Btw, lots of these comment
apply to multiple files and some or all of the patches, I'm not going
to repeat them when it happens again)

+ *   This library is free software; you can redistribute it and/or modify
+ *   it under the terms of the GNU Lesser General Public License as published
+ *   by the Free Software Foundation; either version 2.1 of the License, or
+ *   (at your option) any later version.
+ *
+ *   This library is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
+ *   the GNU Lesser General Public License for more details.
+ *
+ *   You should have received a copy of the GNU Lesser General Public License
+ *   along with this library; if not, write to the Free Software
+ *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA

I strikes me as odd that this is LGPL, but I noticed other files in
fs/cifs/ have this aswell.  We don't ship a copy of the LGPL with the
kernel which is at least against this comment if not even against the
license.  And it'll revert to GPLv2 as part of the kernel anyway,
so it would be much easier if you just declared the code GPLv2.

+
+/* Resolves server name to ip address.
+ * input:
+ * 	unc - server UNC
+ * output:
+ * 	*ip_addr - pointer to server ip, caller responcible for freeing it.
+ * return 0 on success
+ */

This should be a kerneldoc comment.

+static int
+cifs_resolve_server_name_to_ip(const char *unc, char **ip_addr) {

opening curley brace goes on a line of it's own.

+	int rc = -EAGAIN;
+	struct key *rkey;
+	char *name;
+	int len;
+
+	if ((!ip_addr) || (!unc))
+		return -EINVAL;

Useless braces.  Should be:

	if (!ip_addr || !unc)
		return -EINVAL;

+	rkey = request_key(&key_type_cifs_resolver, name, "");
+	if (!IS_ERR(rkey)) {
+		len = strlen(rkey->payload.data);
+		*ip_addr = kmalloc(len+1, GFP_KERNEL);
+		if (*ip_addr) {
+			memcpy(*ip_addr, rkey->payload.data, len);
+			(*ip_addr)[len] = '\0';
+			cFYI(1, ("%s: resolved: %s to %s", __FUNCTION__,
+					rkey->description,
+					*ip_addr
+				));
+			rc = 0;
+		} else {
+			rc = -ENOMEM;
+		}
+		key_put(rkey);
+	} else {
+		cERROR(1, ("%s: unable to resolve: %s", __FUNCTION__, name));
+	}

please use proper goto based unwiding instea of the if-else galore.

+#ifndef _CIFS_DFS_REF_H
+#define _CIFS_DFS_REF_H
+
+#ifdef __KERNEL__
+extern struct key_type key_type_cifs_resolver;
+#endif /* KERNEL */

No need for __KERNEL__ ifdefs here.

+#ifdef CONFIG_CIFS_DFS_UPCALL
+	rc = register_key_type(&key_type_cifs_resolver);
+	if (rc)
+		goto out_unregister_key_type;
+#endif

Instead of the ifdef mess please put helpers like
register_resolver_key() into the conditionally compiled file and stub them
out in the header if the config option is not set.