lkml.org 
[lkml]   [2006]   [Jan]   [27]   [last100]   RSS Feed
Views: [more markup]  [less markup]  [headers]  [forward] 
 
Messages in this thread
/
DateFri, 27 Jan 2006 18:38:28 -0800
FromGreg KH <>
SubjectRe: [PATCH 2.6.12.6-xen] sysfs attributes for xen
> linux-2.6-xen-sparse/arch/xen/kernel/xen_sysfs.c> --- /dev/null	Fri Jan 27 11:48:32 2006> +++ b/linux-2.6-xen-sparse/arch/xen/kernel/xen_sysfs.c	Fri Jan 27 14:28:42 
> 2006> @@ -0,0 +1,73 @@> +/* > +    copyright (c) 2006 IBM Corporation > +    Mike Day <ncmike@us.ibm.com>
Wrong copyright notice as per the IBM lawyers :)

> +> +    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.

Are you sure about the version 2 or later?

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

These two paragraphs are not needed.

> +*/> +> +> +> +#include <linux/config.h>> +#include <linux/kernel.h>> +#include <linux/init.h>> +#include <linux/kobject.h>> +#include <linux/sysfs.h>> +> +#include <asm-xen/xen_sysfs.h>> +> +> +static struct subsystem hypervisor_subsys = {> +	.kset = {> +		.kobj = {> +			 .name = "hypervisor",> +		 },> +	},> +};

No, use the proper macros that define this for you.

> +> +static struct kset xen_kset = {> +> +	.kobj = {> +		.name = "xen",> +	},> +};

Why are you createing a xen kset?  You should not have to do this.

> +> +struct subsystem * > +get_hyper_subsys(void) > +{> +	return &hypervisor_subsys;> +}

What does this do?  Just make the hypervisor subsystem structure global
like the rest of the kernel's subsystems are that need this.

> +> +> +struct kset *> +get_xen_kset(void)> +{> +	return &xen_kset;> +}

Again, not needed.

> +> +int __init> +hyper_sysfs_init(void)> +{> +	int err ;> +	> +	if( 0 ==  (err = subsystem_register(&hypervisor_subsys)) ) {
> +		xen_kset.subsys = &hypervisor_subsys;> +		err = kset_register(&xen_kset);> +	}

Is this the xen coding style?  If so, it's got to change before making
it into mainline...  Please fix this up.

Also, don't use a xen kset.  Make it a subsystem.  Life is much easier
that way.

> +/* xen version info */> +static ssize_t xen_version_show(struct kobject * kobj, 
> +				struct attribute * attr, > +				char *page)

Trailing spaces here, and in a lot of other places in your patch.
Please clean them up (there are automatic tools that do this for you...)

> +{> +	long version;> +	long major, minor;> +	char extra_version[16];> +	> +	if ( (version = HYPERVISOR_xen_version(XENVER_version, NULL)) ) {

Do not do assignments within if statments.  It's generally considered bad
form.  Also the spacing is wrong, but I know you will fix that up for
the rest of the patch too, so I'll just not mention it in every place.

> +		> +		major = version >> 16;
> +		minor = version & 0xff;
> +		if( ! HYPERVISOR_xen_version(XENVER_extraversion, 
> +					    extra_version) ) {
> +			page[PAGE_SIZE - 1] = 0x00;

Not needed.

> +			return snprintf(page, PAGE_SIZE - 1, > +					"xen-%ld.%ld%s\n",> +					major, minor, extra_version);> +		}> +	}> +	return 0;

Why not return an error number if this isn't successful?

> +}> +> +static struct xen_attr xen_ver_attr = {> +	.attr = {> +		.name = "version", > +		.mode = 0444, > +		.owner = THIS_MODULE, > +	},> +	.show = xen_version_show,> +};

Please use the proper macros to create these, do NOT do it by hand.

> +> +static struct kobject xen_ver_obj = {> +	.name = "version",> +};

Wow, a static kobject.  Hint, not a good thing to do for a dynamic
thing.  This is not how you create a new attribute.

> +> +/* xen compile info */> +static ssize_t xen_compile_show(struct kobject * kobj, 
> +				struct attribute * attr, > +				char * page)> +{> +	struct xen_compile_info info;> +	> +	if( 0 == HYPERVISOR_xen_version(XENVER_compile_info, &info) ) {
> +		page[PAGE_SIZE - 1] = 0x00;

I'll not point this out again...

> +		return snprintf(page, PAGE_SIZE - 1, > +				"compiled by %s, using %s, on %s\n", 
> +				info.compile_by, > +				info.compile_date, > +				info.compiler);> +	}> +	return 0;

Nor this...

> +static struct xen_attr xen_compile_attr = {> +	.attr = {> +		.name = "compilation", > +		.mode = 0444, > +		.owner = THIS_MODULE, > +	},> +	.show = xen_compile_show,> +};

Nor this...

> +static struct kobject xen_compile_obj = {> +	.name = "compilation",> +};

Or this...

> linux-2.6-xen-sparse/include/asm-xen/xen_sysfs.h> --- /dev/null	Fri Jan 27 11:48:32 2006> +++ b/linux-2.6-xen-sparse/include/asm-xen/xen_sysfs.h	Fri Jan 27 14:28:42 
> 2006> @@ -0,0 +1,45 @@> +/* > +    copyright (c) 2006 IBM Corporation > +    Mike Day <ncmike@us.ibm.com>> +> +    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> +*/> +> +> +> +#ifndef _XEN_SYSFS_H_> +#define _XEN_SYSFS_H_> +> +#ifdef __KERNEL__ 

Is this really needed?  Would an userspace program ever need this?

> +> +#include <linux/kobject.h>> +#include <linux/sysfs.h>> +#include <linux/module.h>

Not needed for this header file

> +#include <asm-xen/asm/hypercall.h>> +#include <asm-xen/xen-public/version.h>

Nor these.

> +struct xen_attr {> +	struct attribute attr;> +	ssize_t (*show)(struct kobject *, struct attribute *, char *);
> +	ssize_t (*store)(struct kobject *, struct attribute *, char *);
> +};> +> +extern int HYPERVISOR_xen_version(int, void*);

Shouldn't this be declared somewhere else?

> +extern struct subsystem * get_hyper_subsys(void);> +extern struct kset * get_xen_kset(void);

Not needed at all.

Hm, is this file even needed?

thanks,

greg k-h
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2006-01-28 03:40    [from the cache]
©2003-2008