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