Static build and a buglet
Pete Zaitcev
zaitcev at redhat.com
Tue Jan 6 18:13:04 GMT 2004
A little buglet is that afs_cell_create() deals with a local
pointer, but a failure path kfree-s afs_cell_root global.
The fix got entangled into the static build changes though...
Regarding the static build, the key problem is that module
parameter rootcell= is required. It might be possible to
play with cmdline parameters, but it's fugly. I just made
a /proc file. A smaller problem was that afs creates a
transport and opens a socket at init time, so it must
be initialized after networking.
-- Pete
diff -urN -X dontdiff linux-2.6.0/fs/afs/cell.c linux-2.6.0-kafs/fs/afs/cell.c
--- linux-2.6.0/fs/afs/cell.c 2003-10-01 15:18:00.000000000 -0700
+++ linux-2.6.0-kafs/fs/afs/cell.c 2004-01-04 19:11:15.000000000 -0800
@@ -31,11 +31,6 @@
static DECLARE_RWSEM(afs_cells_sem); /* add/remove serialisation */
static afs_cell_t *afs_cell_root;
-static char *rootcell;
-
-MODULE_PARM(rootcell,"s");
-MODULE_PARM_DESC(rootcell,"root AFS cell name and VL server IP addr list");
-
#ifdef AFS_CACHING_SUPPORT
static cachefs_match_val_t afs_cell_cache_match(void *target, const void *entry);
static void afs_cell_cache_update(void *source, void *entry);
@@ -70,8 +65,8 @@
/* allocate and initialise a cell record */
cell = kmalloc(sizeof(afs_cell_t) + strlen(name) + 1,GFP_KERNEL);
if (!cell) {
- _leave(" = -ENOMEM");
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto nomem;
}
memset(cell,0,sizeof(afs_cell_t));
@@ -142,10 +137,14 @@
return 0;
badaddr:
- printk("kAFS: bad VL server IP address: '%s'\n",vllist);
+ printk(KERN_ERR "kAFS: bad VL server IP address: '%s'\n",vllist);
+ /* goto error; */
+
error:
+ kfree(cell);
+ nomem:
up_write(&afs_cells_sem);
- kfree(afs_cell_root);
+ _leave(" = %d", ret);
return ret;
} /* end afs_cell_create() */
@@ -153,29 +152,53 @@
/*
* initialise the cell database from module parameters
*/
-int afs_cell_init(void)
+int afs_cell_init(char *rootcell)
{
char *cp;
+ afs_cell_t *old_root, *new_root;
int ret;
_enter("");
- if (!rootcell) {
- printk("kAFS: no root cell specified\n");
- return -EINVAL;
+ if (rootcell == NULL) {
+ /*
+ * Module is loaded with no parameters, or built statically.
+ * In the future we might initialize cell DB here.
+ */
+ _leave(" = 0 (but no root)");
+ return 0;
}
cp = strchr(rootcell,':');
if (!cp) {
printk("kAFS: no VL server IP addresses specified\n");
+ _leave(" = %d (no colon)", -EINVAL);
return -EINVAL;
}
/* allocate a cell record for the root cell */
*cp++ = 0;
- ret = afs_cell_create(rootcell,cp,&afs_cell_root);
- if (ret==0)
- afs_get_cell(afs_cell_root);
+ ret = afs_cell_create(rootcell,cp,&new_root);
+ if (ret != 0) {
+ _leave(" = %d", ret);
+ return ret;
+ }
+
+ /*
+ * As afs_put_cell takes locks by itself, we have to do
+ * a little gymnastics to be race-free.
+ */
+ write_lock(&afs_cells_lock);
+ while (afs_cell_root) {
+ old_root = afs_cell_root;
+ afs_cell_root = NULL;
+ write_unlock(&afs_cells_lock);
+ afs_put_cell(old_root);
+ write_lock(&afs_cells_lock);
+ }
+ afs_get_cell(new_root);
+ afs_cell_root = new_root;
+ write_unlock(&afs_cells_lock);
_leave(" = %d",ret);
return ret;
@@ -217,9 +240,24 @@
ret = 0;
}
else {
- cell = afs_cell_root;
- afs_get_cell(cell);
- ret = 0;
+ read_lock(&afs_cells_lock);
+
+ if ((cell = afs_cell_root) == NULL) {
+ /*
+ * This should not happen unless user tries to mount
+ * when root cell is not set. Return an impossibly
+ * bizzare errno to alert the user. Things like
+ * ENOENT might be "more appropriate" but they happen
+ * for other reasons.
+ */
+ ret = -EDOM;
+ }
+ else {
+ afs_get_cell(cell);
+ ret = 0;
+ }
+
+ read_unlock(&afs_cells_lock);
}
*_cell = cell;
@@ -403,7 +441,7 @@
/*****************************************************************************/
/*
- * purge in-memory cell database on module unload
+ * purge in-memory cell database on module unload or afs_init() failure
* - the timeout daemon is stopped before calling this
*/
void afs_cell_purge(void)
diff -urN -X dontdiff linux-2.6.0/fs/afs/cell.h linux-2.6.0-kafs/fs/afs/cell.h
--- linux-2.6.0/fs/afs/cell.h 2003-10-01 15:18:00.000000000 -0700
+++ linux-2.6.0-kafs/fs/afs/cell.h 2004-01-04 18:26:07.000000000 -0800
@@ -61,7 +61,7 @@
char name[0]; /* cell name - must go last */
};
-extern int afs_cell_init(void);
+extern int afs_cell_init(char *root_cell);
extern int afs_cell_create(const char *name, char *vllist, struct afs_cell **_cell);
diff -urN -X dontdiff linux-2.6.0/fs/afs/main.c linux-2.6.0-kafs/fs/afs/main.c
--- linux-2.6.0/fs/afs/main.c 2003-10-01 15:18:00.000000000 -0700
+++ linux-2.6.0-kafs/fs/afs/main.c 2004-01-04 20:54:06.000000000 -0800
@@ -33,9 +33,19 @@
static int afs_adding_peer(struct rxrpc_peer *peer);
static void afs_discarding_peer(struct rxrpc_peer *peer);
-module_init(afs_init);
+/*
+ * XXX late_initcall is kludgy. But the only alternative seems to create
+ * a transport upon the first mount, which is worse. Or is it?
+ */
+/* module_init(afs_init); */
+late_initcall(afs_init); /* Must be called after net/ to create socket */
module_exit(afs_exit);
+static char *rootcell;
+
+MODULE_PARM(rootcell,"s");
+MODULE_PARM_DESC(rootcell,"root AFS cell name and VL server IP addr list");
+
MODULE_DESCRIPTION("AFS Client File System");
MODULE_AUTHOR("Red Hat, Inc.");
MODULE_LICENSE("GPL");
@@ -88,7 +98,7 @@
#endif
/* initialise the cell DB */
- ret = afs_cell_init();
+ ret = afs_cell_init(rootcell);
if (ret < 0)
goto error_cache;
diff -urN -X dontdiff linux-2.6.0/fs/afs/proc.c linux-2.6.0-kafs/fs/afs/proc.c
--- linux-2.6.0/fs/afs/proc.c 2003-07-13 20:32:29.000000000 -0700
+++ linux-2.6.0-kafs/fs/afs/proc.c 2004-01-06 12:10:11.000000000 -0800
@@ -44,6 +44,19 @@
.release = seq_release,
};
+static int afs_proc_rootcell_open(struct inode *inode, struct file *file);
+static int afs_proc_rootcell_release(struct inode *inode, struct file *file);
+static ssize_t afs_proc_rootcell_read(struct file *file, char *buf, size_t size, loff_t *_pos);
+static ssize_t afs_proc_rootcell_write(struct file *file, const char *buf, size_t size, loff_t *_pos);
+
+static struct file_operations afs_proc_rootcell_fops = {
+ .open = afs_proc_rootcell_open,
+ .read = afs_proc_rootcell_read,
+ .write = afs_proc_rootcell_write,
+ .llseek = no_llseek,
+ .release = afs_proc_rootcell_release
+};
+
static int afs_proc_cell_volumes_open(struct inode *inode, struct file *file);
static int afs_proc_cell_volumes_release(struct inode *inode, struct file *file);
static void *afs_proc_cell_volumes_start(struct seq_file *p, loff_t *pos);
@@ -128,13 +141,17 @@
p->proc_fops = &afs_proc_cells_fops;
p->owner = THIS_MODULE;
+ p = create_proc_entry("rootcell",0,proc_afs);
+ if (!p)
+ goto error_cells;
+ p->proc_fops = &afs_proc_rootcell_fops;
+ p->owner = THIS_MODULE;
+
_leave(" = 0");
return 0;
-#if 0
error_cells:
remove_proc_entry("cells",proc_afs);
-#endif
error_proc:
remove_proc_entry("fs/afs",NULL);
error:
@@ -248,7 +265,7 @@
/*****************************************************************************/
/*
* handle writes to /proc/fs/afs/cells
- * - to add cells: echo "add <cellname> <IP>[:<IP>][:<IP>]*
+ * - to add cells: echo "add <cellname> <IP>[:<IP>][:<IP>]"
*/
static ssize_t afs_proc_cells_write(struct file *file, const char *buf, size_t size, loff_t *_pos)
{
@@ -313,6 +330,67 @@
/*****************************************************************************/
/*
+ * Stubs for /proc/fs/afs/rootcell
+ */
+static int afs_proc_rootcell_open(struct inode *inode, struct file *file)
+{
+ return 0;
+}
+
+static int afs_proc_rootcell_release(struct inode *inode, struct file *file)
+{
+ return 0;
+}
+
+static ssize_t afs_proc_rootcell_read(struct file *file, char *buf, size_t size, loff_t *_pos)
+{
+ return 0;
+}
+
+/*****************************************************************************/
+/*
+ * handle writes to /proc/fs/afs/rootcell
+ * - to initialize rootcell: echo "cell.name:192.168.231.14"
+ */
+static ssize_t afs_proc_rootcell_write(struct file *file, const char *buf, size_t size, loff_t *_pos)
+{
+ char *kbuf, *s;
+ int ret;
+
+ /* start by dragging the command into memory */
+ if (size<=1 || size>=PAGE_SIZE)
+ return -EINVAL;
+
+ ret = -ENOMEM;
+ kbuf = kmalloc(size+1,GFP_KERNEL);
+ if (!kbuf)
+ goto nomem;
+
+ ret = -EFAULT;
+ if (copy_from_user(kbuf,buf,size)!=0)
+ goto infault;
+ kbuf[size] = 0;
+
+ /* trim to first NL */
+ s = memchr(kbuf,'\n',size);
+ if (s) *s = 0;
+
+ /* determine command to perform */
+ _debug("rootcell=%s",kbuf);
+
+ ret = afs_cell_init(kbuf);
+ if (ret >= 0)
+ ret = size; /* Consume everything, always */
+
+ infault:
+ kfree(kbuf);
+ nomem:
+ _leave(" = %d",ret);
+ return ret;
+}
+
+/*****************************************************************************/
+/*
* initialise /proc/fs/afs/<cell>/
*/
int afs_proc_cell_setup(afs_cell_t *cell)
More information about the linux-afs
mailing list