On Monday January 7, [EMAIL PROTECTED] wrote:
> We've implemented two new NFSD procfs files:
>
> o /proc/fs/nfsd/unlock_ip
> o /proc/fs/nfsd/unlock_filesystem
>
> They are intended to allow admin or user mode script to release NLM
> locks based on either a path name or a server in-bound ip address (ipv4
> for now)
> as;
>
> shell> echo 10.1.1.2 > /proc/fs/nfsd/unlock_ip
> shell> echo /mnt/sfs1 > /proc/fs/nfsd/unlock_filesystem
I'm happy with this interface and the code looks credible, so
Acked-by: NeilBrown <[EMAIL PROTECTED]>
however......
> --- linux-o/fs/nfsd/nfsctl.c 2008-01-04 10:01:08.000000000 -0500
> +++ linux/fs/nfsd/nfsctl.c 2008-01-06 15:27:34.000000000 -0500
> @@ -288,6 +295,56 @@ static ssize_t write_getfd(struct file *
> return err;
> }
>
> +extern __u32 in_aton(const char *str);
Bad. It is "__be32" in linux/inet.h, and the difference an be
important.
Can you just #include <linux/inet.h> ???
> +
> +static
> +ssize_t failover_parse(int where, struct file *file, char *buf, size_t size)
> +{
> + char *fo_path, *mesg;
> + __be32 server_ip[4];
Why '4' ???
Also, fo_path is only sometimes a path, so the name choice could be
confusing. You use "data" in the formal parameters for nfsd_fo_cmd,
which is more idiomatic at least.
Maybe we should have a
union unlock_args {
char *path;
__be32 IPv4;
};
and pass around a pointer to such a union?
If you don't like that I would be happy with a 'void*', but not with a
'char *' called path.
> @@ -717,7 +776,6 @@ static void __exit exit_nfsd(void)
> nfsd4_free_slabs();
> unregister_filesystem(&nfsd_fs_type);
> }
> -
> MODULE_AUTHOR("Olaf Kirch <[EMAIL PROTECTED]>");
> MODULE_LICENSE("GPL");
> module_init(init_nfsd)
Any good reason for removing this blank line?
> +int nlmsvc_fo_match(struct nlm_host *dummy1, struct nlm_host *dummy2)
> +{
> + return 1;
> +}
White space damage. Did you run checkpatch.pl??
> +int
> +nlmsvc_fo_cmd(int cmd, void *datap, int grace_time)
> +{
> + nlm_fo_cmd fo_cmd;
> + int rc=-EINVAL;
> +
> + fo_printk("lockd: nlmsvc_fo_cmd enter, cmd=%d, datap=0x%p, gp=%d\n",
> + cmd, datap, grace_time);
> +
> + fo_cmd.cmd = cmd;
> + fo_cmd.stat = 0;
> + fo_cmd.gp = 0;
> + fo_cmd.datap = datap;
> +
> + /* "if" place holder for NFSD_FO_RESUME */
> + {
> + /* fo_start */
> + rc = nlm_traverse_files((struct nlm_host*) &fo_cmd,
> + nlmsvc_fo_match);
> + fo_printk("nlmsvc_fo_cmd rc=%d, stat=%d\n", rc, fo_cmd.stat);
> + }
> +
> + return rc;
> +}
> +
> +EXPORT_SYMBOL(nlmsvc_fo_cmd);
I think today's convention it to not have a blank line before
EXPORT_SYMBOL. checkpatch.pl should pick this up for you.
> --- linux-o/include/linux/lockd/lockd.h 2008-01-04 10:01:08.000000000
> -0500
> +++ linux/include/linux/lockd/lockd.h 2008-01-06 15:14:55.000000000 -0500
> @@ -39,7 +39,7 @@
> struct nlm_host {
> struct hlist_node h_hash; /* doubly linked list */
> struct sockaddr_in h_addr; /* peer address */
> - struct sockaddr_in h_saddr; /* our address (optional) */
> + struct sockaddr_in h_saddr; /* our address (optional) */
> struct rpc_clnt * h_rpcclnt; /* RPC client to talk to peer */
> char * h_name; /* remote hostname */
> u32 h_version; /* interface version */
This change is purely white-space breakage.
> @@ -214,6 +215,17 @@ void nlmsvc_mark_resources(void);
> void nlmsvc_free_host_resources(struct nlm_host *);
> void nlmsvc_invalidate_all(void);
>
> +/* cluster failover support */
> +
> +typedef struct {
> + int cmd;
> + int stat;
> + int gp;
> + void *datap;
> +} nlm_fo_cmd;
gp??? I guess that means 'grace period'. It isn't used at all in
this patch. Ideally it should only be introduce in the patch which
uses it, but it definitely needs a better name - and preferably a
comment.
NeilBrown
