Re: [ PATCH 2.6.16-rc3-xen 3/3] sysfs: export Xen hypervisor attributes to sysfs

[Greg KH]

No full description explaining why you need this?

No "Signed-off-by:"?

On Tue, Feb 21, 2006 at 09:40:02AM -0500, Mike D. Day wrote:
> # HG changeset patch
> # User mdday@xxxxxxxxxxxxxxxxxxxx
> # Node ID 10c66e0408d1b22db15b8943223f1b6d7713422d
> # Parent  f5f32dc60121c32fab158a814c914aae3b77ba06
> Module that exports Xen Hypervisor attributes to /sys/hypervisor. 
> 
> signed-off-by: Mike D. Day <ncmike@xxxxxxxxxx>
> 
> diff -r f5f32dc60121 -r 10c66e0408d1 
> linux-2.6-xen-sparse/drivers/xen/core/Makefile
> --- a/linux-2.6-xen-sparse/drivers/xen/core/Makefile	Tue Feb 21 08:12:11 
> 2006 -0500

Linewrapped :(

> +++ b/linux-2.6-xen-sparse/drivers/xen/core/Makefile	Tue Feb 21 08:13:00 
> 2006 -0500
> @@ -7,3 +7,4 @@ obj-$(CONFIG_PROC_FS) += xen_proc.o
> obj-$(CONFIG_PROC_FS) += xen_proc.o
> obj-$(CONFIG_NET)     += skbuff.o
> obj-$(CONFIG_SMP)     += smpboot.o
> +obj-$(CONFIG_XEN_SYSFS) += xen_sysfs.o

leading spaces eaten by your email client.  It looks like it was hungry
this morning...


> diff -r f5f32dc60121 -r 10c66e0408d1 
> linux-2.6-xen-sparse/drivers/xen/core/xen_sysfs.c
> --- /dev/null	Thu Jan  1 00:00:00 1970 +0000
> +++ b/linux-2.6-xen-sparse/drivers/xen/core/xen_sysfs.c	Tue Feb 21 
> 08:13:00 2006 -0500
> @@ -0,0 +1,411 @@
> +#include <linux/config.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/kobject.h>
> +#include <linux/sysfs.h>
> +
> +#include <xen/xen_sysfs.h>
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Mike D. Day <ncmike@xxxxxxxxxx>");
> +
> +decl_subsys(hypervisor, NULL, NULL);

This should go into a separate file, along with it's registration, so
that other hypervisors can have access to it, right?

> +struct kset xen_kset = {
> +	.subsys = &hypervisor_subsys,
> +	.kobj = {
> +		.name = "xen",
> +	},
> +};

I thought I asked you last time to not create a "bare" kset.  Any reason
why you didn't like that suggestion?

> +static struct sysfs_ops xen_ops = {
> +
> +	.show = xen_show,
> +};

You will _never_ have a store attribute for xen?  And why are you
passing the attribute field to your show function if you never use it?

> +/* xen version attributes */
> +
> +static ssize_t major_show(struct kobject * kobj,
> +			  struct attribute * attr,
> +			  char * page)
> +{
> +	int version = HYPERVISOR_xen_version(XENVER_version, NULL);
> +	if (version)
> +		return sprintf(page, "%d\n", version >> 16);
> +	return 0;
> +}

Shouldn't you return an error if you could not get the version?
Otherwise your userspace tools will not know something is wrong.

You do this for all of your show attributes...

> +static ssize_t extra_show(struct kobject * kobj,
> +			  struct attribute * attr,
> +			  char * page)
> +{
> +	char extra_ver[XENVER_EXTRAVERSION_LEN];
> +	int err = HYPERVISOR_xen_version(XENVER_extraversion, extra_ver);
> +	if (0 == err)
> +		return sprintf(page, "%s\n", extra_ver);
> +	return 0;
> +}

What's with the (0 == err)?  You don't do that above in the other
functions, so please be consistant.  Putting a value on the left side is
not normal kernel coding style.

> +static struct kobject * v_kobj;
> +
> +int __init
> +xen_version_init(void)
> +{
> +	int err = -ENOMEM;
> +	v_kobj = kcalloc(sizeof(struct kobject), sizeof(char), GFP_KERNEL);

kzalloc please.

> +	if (v_kobj) {
> +		kobject_set_name(v_kobj, "version");
> +		v_kobj->ktype = &xen_version_type;
> +		kobject_init(v_kobj);
> +		v_kobj->dentry = xen_kset.kobj.dentry;
> +		err = sysfs_create_group(v_kobj, &version_group);
> +	}
> +	return err;
> +}
> +
> +
> +static ssize_t compiler_show(struct kobject * kobj,
> +			     struct attribute * attr,
> +			     char * page)
> +{
> +	int err = 0;
> +	struct xen_compile_info * info =
> +		kmalloc(sizeof(struct xen_compile_info), GFP_KERNEL);
> +	if (info ) {
> +		if (0 == HYPERVISOR_xen_version(XENVER_compile_info, info))
> +			err = sprintf(page, "%s\n", info->compiler);

Hey, look a different way to check the return value... Are you sure only
1 person wrote this file, and not 3 different authors?  :)

> +		kfree(info);
> +	}
> +	return err;
> +}
> +

> +static struct attribute_group xen_compilation_group = {
> +	.name = "compilation",
> +	.attrs = xen_compile_attrs,
> +};
> +	

Trailing whitespace.  This is also in other parts of the patch, please
do not do that.

> +static struct kobj_type xen_compilation_type = {
> +	.release = kobj_release,
> +	.sysfs_ops = &xen_ops,
> +	.default_attrs = xen_compile_attrs,
> +};
> +
> +static struct kobject * c_kobj;

I'm all for descriptive variable names, but unfortunatly, this does not
qualify...

> +int __init
> +static xen_compilation_init(void)
> +{
> +	int err = -ENOMEM;
> +	c_kobj = kcalloc(sizeof(struct kobject), sizeof(char), GFP_KERNEL);
> +	if (c_kobj) {
> +		kobject_set_name(c_kobj, "compilation");
> +		c_kobj->ktype = &xen_compilation_type;
> +		kobject_init(c_kobj);
> +		c_kobj->dentry = xen_kset.kobj.dentry;
> +		err = sysfs_create_group(c_kobj, &xen_compilation_group);
> +	}
> +	return err;
> +}

No, just name the attribute group.  Then you don't have to create a
kobject at all.  Same goes for all of the different attribute groups...


> +
> +static void xen_compilation_destroy(void)
> +{
> +	sysfs_remove_group(c_kobj, &xen_compilation_group);
> +	kobject_put(c_kobj);
> +	return;
> +}
> +
> +
> +/* xen properties info */
> +
> +static ssize_t capabilities_show(struct kobject * kobj,
> +				 struct attribute * attr,
> +				 char * page)
> +{
> +	int err = 0;
> +	char * caps = kmalloc(XENVER_CAPABILITIES_INFO_LEN, GFP_KERNEL);
> +	if (caps) {
> +		if (0 == HYPERVISOR_xen_version(XENVER_capabilities, caps))
> +			err = sprintf(page, "%s\n", caps);
> +		kfree(caps);
> +	}
> +	return err;
> +}
> +
> +static struct xen_attr xen_capable = __ATTR_RO(capabilities);
> +
> +static ssize_t changeset_show(struct kobject * kobj,
> +			      struct attribute * attr,
> +			      char * page)
> +{
> +	int err = 0;
> +	char * cset = kmalloc(XENVER_CSET_INFO_LEN, GFP_KERNEL);
> +	if (cset) {
> +		if (0 == HYPERVISOR_xen_version(XENVER_changeset, cset))
> +			err = sprintf(page, "%s\n", cset);
> +		kfree(cset);
> +	}
> +	return err;
> +}
> +
> +static struct xen_attr xen_cset = __ATTR_RO(changeset);
> +
> +static ssize_t virt_start_show(struct kobject * kobj,
> +			       struct attribute * attr,
> +			       char * page)
> +{
> +	int err = 0;
> +	struct xen_platform_parameters * parms =
> +		kmalloc(sizeof(struct xen_platform_parameters), GFP_KERNEL);
> +	if (parms) {
> +		if (0 == HYPERVISOR_xen_version(XENVER_platform_parameters,
> +						parms))
> +			err = sprintf(page, "%lx\n", parms->virt_start);
> +		kfree(parms);
> +	}
> +	return err;
> +}
> +
> +static struct xen_attr xen_virt_start = __ATTR_RO(virt_start);
> +
> +
> +static ssize_t writable_pt_show(struct kobject * kobj,
> +				struct attribute * attr,
> +				char * page)
> +{
> +	int err = 0;
> +	struct xen_feature_info * info =
> +		kmalloc(sizeof(struct xen_feature_info), GFP_KERNEL);
> +	if (info) {
> +		info->submap_idx = XENFEAT_writable_page_tables;
> +		if (0 == HYPERVISOR_xen_version(XENVER_get_features, info))
> +			err = sprintf(page, "%d\n", info->submap);
> +		kfree(info);
> +	}
> +	return err;
> +}
> +
> +static struct xen_attr xen_wpt = __ATTR_RO(writable_pt);
> +
> +static ssize_t writable_dt_show(struct kobject * kobj,
> +				struct attribute * attr,
> +				char * page)
> +{
> +	int err = 0;
> +	struct xen_feature_info * info =
> +		kmalloc(sizeof(struct xen_feature_info), GFP_KERNEL);
> +	if (info) {
> +		info->submap_idx = XENFEAT_writable_descriptor_tables;
> +		if (0 == HYPERVISOR_xen_version(XENVER_get_features, info))
> +			err = sprintf(page, "%d\n", info->submap);
> +		kfree(info);
> +	}
> +	return err;
> +}
> +
> +static struct xen_attr xen_wdt = __ATTR_RO(writable_dt);
> +
> +static ssize_t translated_pm_show(struct kobject * kobj,
> +				  struct attribute * attr,
> +				  char * page)
> +{
> +	int err = 0;
> +	struct xen_feature_info * info =
> +		kmalloc(sizeof(struct xen_feature_info), GFP_KERNEL);
> +	if (info) {
> +		info->submap_idx = XENFEAT_auto_translated_physmap;
> +		if (0 == HYPERVISOR_xen_version(XENVER_get_features, info))
> +			err = sprintf(page, "%d\n", info->submap);
> +		kfree(info);
> +	}
> +	return err;
> +}
> +
> +static struct xen_attr xen_atp = __ATTR_RO(translated_pm);
> +
> +static struct attribute * xen_properties_attrs[] = {
> +	&xen_capable.attr,
> +	&xen_cset.attr,
> +	&xen_virt_start.attr,
> +	&xen_wpt.attr,
> +	&xen_wdt.attr,
> +	&xen_atp.attr,
> +	NULL
> +};
> +
> +static struct attribute_group xen_properties_group = {
> +	.name = "properties",
> +	.attrs = xen_properties_attrs,
> +};
> +
> +static struct kobj_type xen_properties_type = {
> +	.release = kobj_release,
> +	.sysfs_ops = &xen_ops,
> +	.default_attrs = xen_properties_attrs,
> +};
> +
> +static struct kobject * p_kobj;
> +
> +static int __init
> +xen_properties_init(void)
> +{
> +	int err = -ENOMEM;
> +	p_kobj = kcalloc(sizeof(struct kobject), sizeof(char), GFP_KERNEL);
> +	if (p_kobj) {
> +		kobject_set_name(p_kobj, "properties");
> +		p_kobj->ktype = &xen_properties_type;
> +		kobject_init(p_kobj);
> +		p_kobj->dentry = xen_kset.kobj.dentry;
> +		err = sysfs_create_group(p_kobj, &xen_properties_group);
> +	}
> +	return err;
> +}
> +	
> +static void xen_properties_destroy(void)
> +{
> +	sysfs_remove_group(p_kobj, &xen_properties_group);
> +	kobject_put(p_kobj);
> +}
> +
> +
> +static int __init
> +hyper_sysfs_init(void)
> +{
> +	__label__ out;

What is "__label__"?

> +	
> +	int err = subsystem_register(&hypervisor_subsys);
> +	if (err)
> +		goto out;
> +	err = kset_register(&xen_kset);
> +	if (err)
> +		goto out;
> +	err = xen_version_init();
> +	if (err)
> +		goto out;
> +	err = xen_compilation_init();
> +	if (err)
> +		goto out;
> +	err = xen_properties_init();
> +
> +out:
> +	return err;

If you have an error, you do not back out the registration of any of the
other attributes.  So, why have any error handling at all?

> +static void hyper_sysfs_exit(void)
> +{
> + 	xen_properties_destroy();
> +	xen_compilation_destroy();
> +	xen_version_destroy();
> +	kset_unregister(&xen_kset);
> +	subsystem_unregister(&hypervisor_subsys);
> +}
> +
> +
> +module_init(hyper_sysfs_init);
> +module_exit(hyper_sysfs_exit);
> +
> diff -r f5f32dc60121 -r 10c66e0408d1 
> linux-2.6-xen-sparse/include/xen/xen_sysfs.h
> --- /dev/null	Thu Jan  1 00:00:00 1970 +0000
> +++ b/linux-2.6-xen-sparse/include/xen/xen_sysfs.h	Tue Feb 21 08:13:00 
> 2006 -0500
> @@ -0,0 +1,30 @@
> +/* 
> +    copyright (c) 2006 IBM Corporation 
> +    Authored by: Mike D. 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 version 2 as 
> +    published by the Free Software Foundation.
> +*/
> +
> +
> +
> +#ifndef _XEN_SYSFS_H_
> +#define _XEN_SYSFS_H_
> +
> +#include <linux/kobject.h>
> +#include <linux/sysfs.h>
> +#include <linux/module.h>
> +#include <asm/hypercall.h>
> +#include <xen/interface/version.h>

Why does this header file depend on the module, hypercall and version
header files?

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

Why export this structure?
Why even have this header file at all?

> +
> +extern int HYPERVISOR_xen_version(int, void*);

You don't have this in some xen header file?  Shouldn't it go next to
all of the other HYPERVISOR calls?

Care to try again?

thanks,

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