[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] [RFC] sysfs support for xen linux
* Mike D. Day (ncmike@xxxxxxxxxx) wrote: > This patch is the first step toward instrumenting xen through sysfs, and > toward migrating the /proc/xen files to /sys/xen. > > The major component is a set of kernel functions that hopefully make > adding files to /sys/xen as easy as adding files to /proc/xen. A > smaller file adds xen version information by creating a file under > /sys/xen/version. > > I am looking for feedback on the approach and usefulness of the sysfs > support functions. The next step is to add support for sysfs binary > files and to experiment with implementing /proc/xen/privcmd as > /sysfs/xen/privcmd You're re-inventing the wheel here. The infrastructure is there so that you don't have to create your own. I think you need to back up and consider the requirements again. E.g. exporting version should be _tiny_. > # HG changeset patch > # User mdday@xxxxxxxxxxxxxxxxxxxxx > # Node ID cec2fc0a07c611023e096cf3496d948aa39c1342 > # Parent c08884b412da24dd4c05d36fdff408f4433bd865 > # Parent da7873110bbb8b55d9adb9111d100e209fc49ee6 > signed-off-by Mike Day <ncmike@xxxxxxxxxx> > > Stage support for xen to export information using sysfs. Make it just as easy > to add a /sys/xen/ file as it is to add a /proc/xen file currently. Starting > by exporting xen version information in /sys/xen/version. > > diff -r c08884b412da -r cec2fc0a07c6 > linux-2.6-xen-sparse/arch/xen/kernel/xen_sysfs.c > --- /dev/null Mon Jan 9 21:55:13 2006 > +++ b/linux-2.6-xen-sparse/arch/xen/kernel/xen_sysfs.c Mon Jan 9 > 23:07:04 2006 > @@ -0,0 +1,698 @@ > +/* > + copyright (c) 2006 IBM Corporation > + Mike Day <ncmike@xxxxxxxxxx> > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 2 of the License, or > + (at your option) any later version. > + > + This program 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 General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program; if not, write to the Free Software > + Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA > +*/ > + > + > +#include <linux/config.h> > +#include <linux/kernel.h> > +#include <linux/init.h> > +#include <linux/kobject.h> > +#include <linux/sysfs.h> > +#include <linux/module.h> > +#include <linux/string.h> > +#include <linux/types.h> > +#include <asm/atomic.h> > +#include <asm/semaphore.h> > +#include <asm-generic/bug.h> > + > +#ifdef DEBUG > +#define DPRINTK(fmt, args...) printk(KERN_DEBUG "xen_sysfs: ", fmt, ## > args) > +#else > +#define DPRINTK(fmt, args...) > +#endif pr_debug > +#ifndef BOOL > +#define BOOL int > +#endif > + > +#ifndef FALSE > +#define FALSE 0 > +#endif > + > +#ifndef TRUE > +#define TRUE 1 > +#endif > +#ifndef NULL > +#define NULL 0 > +#endif unecessary, drop all this > +#define __sysfs_ref__ what's this for? > +struct xen_sysfs_object; > + > +struct xen_sysfs_attr > +{ > + struct bin_attribute attr; > + ssize_t (*show)(void *, char *) ; > + ssize_t (*store)(void *, const char *, size_t) ; > + ssize_t (*read)(void *, char *, loff_t, size_t ); > + ssize_t (*write)(void *, char *, loff_t, size_t) ; > +}; > + > + > + > +/* flags bits */ > +#define XEN_SYSFS_UNINITIALIZED 0x00 > +#define XEN_SYSFS_CHAR_TYPE 0x01 > +#define XEN_SYSFS_BIN_TYPE 0x02 > +#define XEN_SYSFS_DIR_TYPE 0x04 > +#define XEN_SYSFS_LINKED 0x08 > +#define XEN_SYSFS_UNLINKED 0x10 > +#define XEN_SYSFS_LINK_TYPE 0x11 > + > + > +struct xen_sysfs_object > +{ > + struct list_head list; > + int flags; > + struct kobject kobj; > + struct xen_sysfs_attr attr; > + char * path; > + struct list_head children; > + struct xen_sysfs_object * parent; > + atomic_t refcount; This looks like you're creating your own tree structure. kobjects already handle this. > + void * user_data; > + void (*user_data_release)(void *); > + void (*destroy)(struct xen_sysfs_object *); > +}; > + > + > +static __sysfs_ref__ struct xen_sysfs_object * > +find_object(struct xen_sysfs_object * obj, const char * path); > + > + > +static __sysfs_ref__ struct xen_sysfs_object * > +new_sysfs_object(const char * path, > + int type, > + int mode, > + ssize_t (*show)(void *, char *), > + ssize_t (*store)(void *, const char *, size_t), > + ssize_t (*read)(void *, char *, loff_t, size_t), > + ssize_t (*write)(void *, char *, loff_t, size_t), > + void * user_data, > + void (* user_data_release)(void *)) ; > + > +static void destroy_sysfs_object(struct xen_sysfs_object * obj); > +static __sysfs_ref__ struct xen_sysfs_object * __find_parent(const char * > path) ; > +static __sysfs_ref__ int __add_child(struct xen_sysfs_object *parent, > + struct xen_sysfs_object *child); > +static void remove_child(struct xen_sysfs_object *child); > +static void get_object(struct xen_sysfs_object *); > +static int put_object(struct xen_sysfs_object *, > + void (*)(struct xen_sysfs_object *)); > + > + > +/* Is A == B ? */ > +#define streq(a,b) (strcmp((a),(b)) == 0) > + > +/* Does A start with B ? */ > +#define strstarts(a,b) (strncmp((a),(b),strlen(b)) == 0) these are typically done open coded... > +#define __sysfs_ref__ debugging? > +#define XEN_SYSFS_ATTR(_name, _mode, _show, _store) \ > + struct xen_sysfs_attr xen_sysfs_attr_##_name = __ATTR(_name, _mode, > _show, _store) > + > +#define __XEN_KOBJ(_parent, _dentry, _ktype) \ > + { \ > + .k_name = NULL, \ > + .parent = _parent, \ > + .dentry = _dentry, \ > + .ktype = _ktype, \ > + } > + > +static struct semaphore xen_sysfs_mut = __MUTEX_INITIALIZER(xen_sysfs_mut); > +static inline int > +sysfs_down(struct semaphore * mut) > +{ > + int err; > + do { > + err = down_interruptible(mut); > + } while ( err && err == -EINTR ); > + return err; > +} What's the point of using down_interruptible if you can't really interrupt the call flow? > +#define sysfs_up(mut) up(mut) > +#define to_xen_attr(_attr) container_of(_attr, struct xen_sysfs_attr, > attr.attr) > +#define to_xen_obj(_xen_attr) container_of(_xen_attr, struct > xen_sysfs_object, attr) > + > +static ssize_t > +xen_sysfs_show(struct kobject * kobj, struct attribute * attr, char * buf) > +{ > + struct xen_sysfs_attr * xen_attr = to_xen_attr(attr); > + struct xen_sysfs_object * xen_obj = to_xen_obj(xen_attr); > + if(xen_attr->show) > + return xen_attr->show(xen_obj->user_data, buf); > + return 0; > +} > + > +static ssize_t > +xen_sysfs_store(struct kobject * kobj, struct attribute * attr, > + const char *buf, size_t count) > +{ > + struct xen_sysfs_attr * xen_attr = to_xen_attr(attr); > + struct xen_sysfs_object * xen_obj = to_xen_obj(xen_attr); > + if(xen_attr->store) > + return xen_attr->store(xen_obj->user_data, buf, count) ; > + return 0; > +} > + > +#define to_xen_obj_bin(_kobj) container_of(_kobj, struct xen_sysfs_object, > kobj) > + > +static ssize_t > +xen_sysfs_read(struct kobject *kobj, char * buf, loff_t offset, size_t size) > +{ > + struct xen_sysfs_object * xen_obj = to_xen_obj_bin(kobj); > + if(xen_obj->attr.read) > + return xen_obj->attr.read(xen_obj->user_data, buf, offset, > size); > + return 0; > +} > + > + > +static ssize_t > +xen_sysfs_write(struct kobject *kobj, char * buf, loff_t offset, size_t size) > +{ > + struct xen_sysfs_object * xen_obj = to_xen_obj_bin(kobj); > + if (xen_obj->attr.write) > + return xen_obj->attr.write(xen_obj->user_data, buf, offset, > size); > + if(size == 0 ) > + return PAGE_SIZE; > + > + return size; > +} > + > +static struct sysfs_ops xen_sysfs_ops = { > + .show = xen_sysfs_show, > + .store = xen_sysfs_store, > +}; > + > +static struct kobj_type xen_kobj_type = { > + .release = NULL, > + .sysfs_ops = &xen_sysfs_ops, > + .default_attrs = NULL, > +}; > + > + > +/* xen sysfs root entry */ > +static struct xen_sysfs_object xen_root = { > + .flags = 0, > + .kobj = { > + .k_name = NULL, > + .parent = NULL, > + .dentry = NULL, > + .ktype = &xen_kobj_type, > + }, > + .attr = { > + .attr = { > + .attr = { > + .name = NULL, > + .mode = 0775, > + }, > + > + }, > + .show = NULL, > + .store = NULL, > + .read = NULL, > + .write = NULL, > + }, > + .path = __stringify(/sys/xen), > + .list = LIST_HEAD_INIT(xen_root.list), > + .children = LIST_HEAD_INIT(xen_root.children), > + .parent = NULL, > +}; > + > +/* xen sysfs path functions */ > + > +static BOOL > +valid_chars(const char *path) > +{ > + if( ! strstarts(path, "/sys/xen") ) OK, that's a serious problem. You should not have pathnames here at all. > + return FALSE; > + if(strstr(path, "//")) > + return FALSE; > + return (strspn(path, > + "ABCDEFGHIJKLMNOPQRSTUVWXYZ" > + "abcdefghijklmnopqrstuvwxyz" > + "0123456789-/_@~$") == strlen(path)); eek > +} > + > + > +/* return value must be kfree'd */ > +static char * > +dup_path(const char *path) > +{ > + char * ret; > + int len; > + BUG_ON( ! path ); > + > + if( FALSE == valid_chars(path) ) { > + return NULL; > + } > + > + len = strlen(path) + 1; > + ret = kcalloc(len - 1, sizeof(char), GFP_KERNEL); (despite the fact that this shouldn't be necessary...s/kcalloc/kzalloc/) _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxx http://lists.xensource.com/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |