WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] [PATCH] [RFC] sysfs support for xen linux

To: "Mike D. Day" <ncmike@xxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH] [RFC] sysfs support for xen linux
From: Chris Wright <chrisw@xxxxxxxxxxxx>
Date: Wed, 11 Jan 2006 17:44:45 -0800
Cc: xen-devel@xxxxxxxxxxxxxxxxxxx
Delivery-date: Thu, 12 Jan 2006 10:23:03 +0000
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <43C2F324.4020407@xxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <43C2F324.4020407@xxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mutt/1.4.2.1i
* 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