Creates /sys/hypervisor/xen and populates that dir with xen version, changeset, compilation, and capabilities info. Intended for the xen merge tree and later upstream.
# HG changeset patch
# User [email protected]
# Node ID 9a9f2a5f087c97186bd43561b90f30510413a3e2
# Parent 2e82fd7a69212955b75c6adaed4ae2a58ae45399
add /sys/hypervisor/xen to sysfs and populate with xen version attributes.
signed-off-by: Mike D. Day <[email protected]>
diff -r 2e82fd7a6921 -r 9a9f2a5f087c linux-2.6-xen-sparse/arch/xen/kernel/Makefile
--- a/linux-2.6-xen-sparse/arch/xen/kernel/Makefile Fri Jan 27 11:48:32 2006
+++ b/linux-2.6-xen-sparse/arch/xen/kernel/Makefile Fri Jan 27 14:28:42 2006
@@ -16,3 +16,4 @@
obj-$(CONFIG_PROC_FS) += xen_proc.o
obj-$(CONFIG_NET) += skbuff.o
obj-$(CONFIG_SMP) += smpboot.o
+obj-$(CONFIG_SYSFS) += xen_sysfs.o xen_sysfs_version.o
diff -r 2e82fd7a6921 -r 9a9f2a5f087c 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 <[email protected]>
+
+ 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 <asm-xen/xen_sysfs.h>
+
+
+static struct subsystem hypervisor_subsys = {
+ .kset = {
+ .kobj = {
+ .name = "hypervisor",
+ },
+ },
+};
+
+static struct kset xen_kset = {
+
+ .kobj = {
+ .name = "xen",
+ },
+};
+
+struct subsystem *
+get_hyper_subsys(void)
+{
+ return &hypervisor_subsys;
+}
+
+
+struct kset *
+get_xen_kset(void)
+{
+ return &xen_kset;
+}
+
+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);
+ }
+ return err;
+}
+
+arch_initcall(hyper_sysfs_init);
+EXPORT_SYMBOL_GPL(get_hyper_subsys);
+EXPORT_SYMBOL_GPL(get_xen_kset);
diff -r 2e82fd7a6921 -r 9a9f2a5f087c linux-2.6-xen-sparse/arch/xen/kernel/xen_sysfs_version.c
--- /dev/null Fri Jan 27 11:48:32 2006
+++ b/linux-2.6-xen-sparse/arch/xen/kernel/xen_sysfs_version.c Fri Jan 27 14:28:42 2006
@@ -0,0 +1,194 @@
+/*
+ copyright (c) 2006 IBM Corporation
+ Mike Day <[email protected]>
+
+ 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/types.h>
+#include <asm/page.h>
+
+#include <asm-xen/xen-public/dom0_ops.h>
+#include <asm-xen/xen-public/version.h>
+#include <asm-xen/asm/hypercall.h>
+#include <asm-xen/xen_sysfs.h>
+
+/* xen version info */
+static ssize_t xen_version_show(struct kobject * kobj,
+ struct attribute * attr,
+ char *page)
+{
+ long version;
+ long major, minor;
+ char extra_version[16];
+
+ if ( (version = HYPERVISOR_xen_version(XENVER_version, NULL)) ) {
+
+ major = version >> 16;
+ minor = version & 0xff;
+ if( ! HYPERVISOR_xen_version(XENVER_extraversion,
+ extra_version) ) {
+ page[PAGE_SIZE - 1] = 0x00;
+ return snprintf(page, PAGE_SIZE - 1,
+ "xen-%ld.%ld%s\n",
+ major, minor, extra_version);
+ }
+ }
+ return 0;
+}
+
+static struct xen_attr xen_ver_attr = {
+ .attr = {
+ .name = "version",
+ .mode = 0444,
+ .owner = THIS_MODULE,
+ },
+ .show = xen_version_show,
+};
+
+static struct kobject xen_ver_obj = {
+ .name = "version",
+};
+
+/* 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;
+ return snprintf(page, PAGE_SIZE - 1,
+ "compiled by %s, using %s, on %s\n",
+ info.compile_by,
+ info.compile_date,
+ info.compiler);
+ }
+ return 0;
+}
+
+static struct xen_attr xen_compile_attr = {
+ .attr = {
+ .name = "compilation",
+ .mode = 0444,
+ .owner = THIS_MODULE,
+ },
+ .show = xen_compile_show,
+};
+
+static struct kobject xen_compile_obj = {
+ .name = "compilation",
+};
+
+/* xen changeset info */
+static ssize_t xen_cset_show(struct kobject * kobj,
+ struct attribute * attr,
+ char * page)
+{
+ char info[64];
+
+ if( 0 == HYPERVISOR_xen_version(XENVER_changeset, &info) ) {
+ page[PAGE_SIZE - 1] = 0x00;
+ return snprintf(page, PAGE_SIZE - 1,
+ "%s\n", info);
+ }
+ return 0;
+}
+
+static struct xen_attr xen_cset_attr = {
+ .attr = {
+ .name = "changeset",
+ .mode = 0444,
+ .owner = THIS_MODULE,
+ },
+ .show = xen_cset_show,
+};
+
+static struct kobject xen_cset_obj = {
+ .name = "changeset",
+};
+
+
+/* xen capabilities info */
+static ssize_t xen_cap_show(struct kobject * kobj,
+ struct attribute * attr,
+ char * page)
+{
+ char info[1024];
+
+ if( 0 == HYPERVISOR_xen_version(XENVER_capabilities, &info) ) {
+ page[PAGE_SIZE - 1] = 0x00;
+ return snprintf(page, PAGE_SIZE - 1,
+ "%s\n", info);
+ }
+ return 0;
+}
+
+static struct xen_attr xen_cap_attr = {
+ .attr = {
+ .name = "capabilities",
+ .mode = 0444,
+ .owner = THIS_MODULE,
+ },
+ .show = xen_cap_show,
+};
+
+static struct kobject xen_cap_obj = {
+ .name = "capabilities",
+};
+
+int __init
+sysfs_xen_version_init(void)
+{
+ __label__ out;
+
+ struct kset * parent = get_xen_kset();
+ if ( parent != NULL ) {
+ kobject_init(&xen_ver_obj);
+ xen_ver_obj.parent = &parent->kobj;
+ xen_ver_obj.dentry = parent->kobj.dentry;
+ kobject_get(&parent->kobj);
+ if ( sysfs_create_file(&xen_ver_obj, &xen_ver_attr.attr) )
+ goto out;
+
+ kobject_init(&xen_compile_obj);
+ xen_compile_obj.parent = &parent->kobj;
+ xen_compile_obj.dentry = parent->kobj.dentry;
+ kobject_get(&parent->kobj);
+ if( sysfs_create_file(&xen_compile_obj, &xen_compile_attr.attr) )
+ goto out;
+
+ kobject_init(&xen_cset_obj);
+ xen_cset_obj.parent = &parent->kobj;
+ xen_cset_obj.dentry = parent->kobj.dentry;
+ kobject_get(&parent->kobj);
+ if( sysfs_create_file(&xen_cset_obj, &xen_cset_attr.attr) )
+ goto out;
+
+ kobject_init(&xen_cap_obj);
+ xen_cap_obj.parent = &parent->kobj;
+ xen_cap_obj.dentry = parent->kobj.dentry;
+ kobject_get(&parent->kobj);
+ return sysfs_create_file(&xen_cap_obj, &xen_cap_attr.attr);
+ }
+out:
+ return 1;
+}
+
+device_initcall(sysfs_xen_version_init);
diff -r 2e82fd7a6921 -r 9a9f2a5f087c 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 <[email protected]>
+
+ 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__
+
+#include <linux/kobject.h>
+#include <linux/sysfs.h>
+#include <linux/module.h>
+#include <asm-xen/asm/hypercall.h>
+#include <asm-xen/xen-public/version.h>
+
+
+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*);
+extern struct subsystem * get_hyper_subsys(void);
+extern struct kset * get_xen_kset(void);
+
+#endif /* __KERNEL__ */
+#endif /* _XEN_SYSFS_H_ */
--
On Fri, Jan 27, 2006 at 09:20:11PM -0500, Mike D. Day wrote:
> Creates /sys/hypervisor/xen and populates that dir with xen version,
> changeset, compilation, and capabilities info. Intended for the xen merge
> tree and later upstream.
> # HG changeset patch
> # User [email protected]
> # Node ID 9a9f2a5f087c97186bd43561b90f30510413a3e2
> # Parent 2e82fd7a69212955b75c6adaed4ae2a58ae45399
> add /sys/hypervisor/xen to sysfs and populate with xen version attributes.
>
> signed-off-by: Mike D. Day <[email protected]>
"Signed-off-by:" please.
Also, a valid email address is required here.
>
> diff -r 2e82fd7a6921 -r 9a9f2a5f087c
> linux-2.6-xen-sparse/arch/xen/kernel/Makefile
> --- a/linux-2.6-xen-sparse/arch/xen/kernel/Makefile Fri Jan 27 11:48:32
> 2006
> +++ b/linux-2.6-xen-sparse/arch/xen/kernel/Makefile Fri Jan 27 14:28:42
> 2006
The patch is linewrapped, and all leading spaces were eaten by someone
for dinner.
Please try resending it in a format that can be applied.
thanks,
greg k-h
> 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 <[email protected]>
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 <[email protected]>
> +
> + 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
Comments inlined.
Mike D. Day wrote:
> Creates /sys/hypervisor/xen and populates that dir with xen version,
> changeset, compilation, and capabilities info. Intended for the xen
> merge tree and later upstream.
<snip>
> + if( 0 == (err = subsystem_register(&hypervisor_subsys)) ) {
This formatting is off. You want a space after the if and no space
after the (. See CodingStyle. It could be your mailer (it munged the
newlines) but it appears you've got 4 space tabs? Linux style is 8.
> + if( ! HYPERVISOR_xen_version(XENVER_extraversion,
> + extra_version) ) {
> + page[PAGE_SIZE - 1] = 0x00;
> + return snprintf(page, PAGE_SIZE - 1, +
> "xen-%ld.%ld%s\n",
> + major, minor, extra_version);
snprintf takes into account the terminating byte so you don't have to.
<snip>
> +/* 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;
> + return snprintf(page, PAGE_SIZE - 1, +
> "compiled by %s, using %s, on %s\n", + info.compile_by,
> + info.compile_date, + info.compiler);
> + }
> + return 0;
> +}
I'm not the best person to speak to this but I'm pretty sure sysfs
prefers single values per file so you probably want to split this up
into compile_by, compile_date, compiler.
> +int __init
> +sysfs_xen_version_init(void)
> +{
> + __label__ out;
> +
> + struct kset * parent = get_xen_kset();
> + if ( parent != NULL ) {
> + kobject_init(&xen_ver_obj);
> + xen_ver_obj.parent = &parent->kobj;
> + xen_ver_obj.dentry = parent->kobj.dentry;
> + kobject_get(&parent->kobj);
> + if ( sysfs_create_file(&xen_ver_obj, &xen_ver_attr.attr) )
> + goto out;
I reckon you want to use default attributes here instead of immediately
calling sysfs_create_file().
Regards,
Anthony Liguori
On Fri, Jan 27, 2006 at 06:38:28PM -0800, Greg KH wrote:
> > +
> > +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.
No, this is not coding style we use for linux related files.
The patch really need to be fix ...
--
Vincent Hanquez
This looks much more sound than the /proc version. Nice work.
On Fri, 2006-01-27 at 21:20 -0500, Mike D. Day wrote:
> +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);
> + }
> + return err;
> +}
As Greg said, we like to see assignments taken out of 'if' statements.
I also like to see the main flow of code stay at the top level of a
function, with _exceptions_ being the things that go inside of 'if'
blocks. I think it makes functions much easier to follow, but it's
completely personal preference. Something like this, maybe?
hyper_sysfs_init(void)
{
int err ;
err = subsystem_register(&hypervisor_subsys);
if(err)
return err;
xen_kset.subsys = &hypervisor_subsys;
err = kset_register(&xen_kset);
return err;
}
> +/* xen version info */
> +static ssize_t xen_version_show(struct kobject * kobj,
> + struct attribute * attr,
> + char *page)
> +{
> + long version;
> + long major, minor;
> + char extra_version[16];
> +
> + if ( (version = HYPERVISOR_xen_version(XENVER_version, NULL)) ) {
> +
> + major = version >> 16;
> + minor = version & 0xff;
> + if( ! HYPERVISOR_xen_version(XENVER_extraversion,
> + extra_version) ) {
> + page[PAGE_SIZE - 1] = 0x00;
> + return snprintf(page, PAGE_SIZE - 1,
> + "xen-%ld.%ld%s\n",
> + major, minor, extra_version);
> + }
> + }
> + return 0;
> +}
What are the actual types of the values that come back from the
HYPERVISOR_xen_version(XENVER_version, NULL)
call? If they are really 8-bit or 16-bit values, it might be nice to
call that out and use some of the kernel types like u8 or u16. In fact,
it might even be worth it to have a function wrap that up.
Silly idea: you _could_ have separate files for the major and minor.
Are they something that a userspace program might commonly have to parse
out? Is the patch trying to save a potential hcall by outputting them
both at once?
> +/* 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;
> + return snprintf(page, PAGE_SIZE - 1,
> + "compiled by %s, using %s, on %s\n",
> + info.compile_by,
> + info.compile_date,
> + info.compiler);
> + }
> + return 0;
> +}
I think this one breaks the "one value per file" sysfs rule. Perhaps
instead of 'cat compilation' you need:
greo . compilation/*
Where compilation contains:
|-- compilation
| |-- user
| |-- date
| |-- compiler
> +/* xen capabilities info */
> +static ssize_t xen_cap_show(struct kobject * kobj,
> + struct attribute * attr,
> + char * page)
> +{
> + char info[1024];
> +
> + if( 0 == HYPERVISOR_xen_version(XENVER_capabilities, &info) ) {
> + page[PAGE_SIZE - 1] = 0x00;
> + return snprintf(page, PAGE_SIZE - 1,
> + "%s\n", info);
> + }
> + return 0;
> +}
Where does that 1024 come from? Is it a guarantee from Xen that it will
never fill more than 1k? I know it is a long shot, but what if the page
size is less than 1k? Would this function have strange results?
Also, please don't declare large variables on the stack. If you really
need a buffer that large, simply dynamically allocate it. We have a
really speedy allocator.
> +int __init
> +sysfs_xen_version_init(void)
> +{
> + __label__ out;
> +
> + struct kset * parent = get_xen_kset();
> + if ( parent != NULL ) {
> + kobject_init(&xen_ver_obj);
> + xen_ver_obj.parent = &parent->kobj;
> + xen_ver_obj.dentry = parent->kobj.dentry;
> + kobject_get(&parent->kobj);
> + if ( sysfs_create_file(&xen_ver_obj, &xen_ver_attr.attr) )
> + goto out;
...
> +out:
> + return 1;
> +}
Instead of embedding all of the kobject() initializations, this could
also just do the following:
if ( parent == NULL )
goto out;
kobject_init(&xen_ver_obj);
...
I know the kset stuff might be going away, but you might run into a
similar construct later down the road.
There are quite a few references to PAGE_SIZE in the patch. I think
most of them have to do with trying to make sure that the buffer
returned to the sysfs functions is NULL-terminated. I'm not sure that
is really an issue.
Many sysfs users simply sprintf() right into the buffer, as long as
they're writing reasonably short stuff. We assume that we're not going
to overflow, especially with single integer values. This makes the code
quite a bit more simple.
If overflow is a real worry, we might want to come up with a specialized
sysfs sprintf which encapsulates the PAGE_SIZE restriction. It seems a
little silly to have each driver going through all this trouble of
worrying about buffer sizing and NULL termination.
-- Dave
Dave Hansen wrote:
> What are the actual types of the values that come back from the
>
> HYPERVISOR_xen_version(XENVER_version, NULL)
> call? If they are really 8-bit or 16-bit values, it might be nice to
> call that out and use some of the kernel types like u8 or u16. In fact,
> it might even be worth it to have a function wrap that up.
return is int, but the ranges are small enough for a u8, so a wrap might be good. ret == 0 is ESUCCESS _unless_ you are calling HYPERVISOR_xen_version, in which case ret == version.
> Silly idea: you _could_ have separate files for the major and minor.
> Are they something that a userspace program might commonly have to parse
> out? Is the patch trying to save a potential hcall by outputting them
> both at once?
no, this hypercall is not in any performance path and it also returns major ver and minor ver embedded into the returned int. Good suggestion and no problem calling repeatedly (within reason).
> Where does that 1024 come from? Is it a guarantee from Xen that it will
> never fill more than 1k? I know it is a long shot, but what if the page
> size is less than 1k? Would this function have strange results?
Per the xen headers, this particular hcall option returns a typedef char[1024] thingy_t (which is simply a char [1024] in the patch). Yes, if the page size is < 1024 there is a problem. So a check against PAGE_SIZE may be prudent.
I'm rewriting based on Greg's and your feedback,
thanks again,
Mike
--
On Mon, 2006-01-30 at 11:58 -0500, Mike D. Day wrote:
> Dave Hansen wrote:
> > Where does that 1024 come from? Is it a guarantee from Xen that it will
> > never fill more than 1k? I know it is a long shot, but what if the page
> > size is less than 1k? Would this function have strange results?
>
> Per the xen headers, this particular hcall option returns a typedef
> char[1024] thingy_t (which is simply a char [1024] in the patch). Yes,
> if the page size is < 1024 there is a problem. So a check against
> PAGE_SIZE may be prudent.
I was just looking, and noticed that there is a check in the sysfs
->show() code that checks for return values greater than PAGE_SIZE and
BUG()s, so you at least won't get silent corruption. That is probably
good enough to not worry about it.
In the final version, there will be available Xen headers, and the patch
won't need the open-coded 1024?
-- Dave
Dave Hansen wrote:
> In the final version, there will be available Xen headers, and the patch
> won't need the open-coded 1024?
Good question, I need some advice. The Xen hcall headers get soft-linked into every paravirtualized OS tree: linux, bsd, solaris, etc. In linux right now the xen version.h shows up as /include/asm-xen/version.h.
This file uses typedefs for every important parameter. For example, typedef char [1024] xen_capabilities_info_t;.
But as Greg says TYPEDEFS ARE EVIL.
Last resort would be to use the funky gcc #include_next to override the xen hcall headers with a linux-specific hcall headers. But I don't know if that would be cool with lkml either.
So, advice would be welcome :).
thanks,
Mike
--
On Mon, Jan 30, 2006 at 12:17:17PM -0500, Mike D. Day wrote:
> Dave Hansen wrote:
> >In the final version, there will be available Xen headers, and the patch
> >won't need the open-coded 1024?
>
> Good question, I need some advice. The Xen hcall headers get soft-linked
> into every paravirtualized OS tree: linux, bsd, solaris, etc. In linux
> right now the xen version.h shows up as /include/asm-xen/version.h.
>
> This file uses typedefs for every important parameter. For example, typedef
> char [1024] xen_capabilities_info_t;.
> But as Greg says TYPEDEFS ARE EVIL.
Then just make it a structure. All the operating systems will be able
to handle that just fine :)
thanks,
greg k-h
On Mon, 2006-01-30 at 12:17 -0500, Mike D. Day wrote:
> Dave Hansen wrote:
> > In the final version, there will be available Xen headers, and the
> patch
> > won't need the open-coded 1024?
>
> Good question, I need some advice. The Xen hcall headers get
> soft-linked into every paravirtualized OS tree: linux, bsd, solaris,
> etc. In linux right now the xen version.h shows up
> as /include/asm-xen/version.h.
But, you can #include it from any old Linux file and not care, right?
> This file uses typedefs for every important parameter. For example,
> typedef char [1024] xen_capabilities_info_t;.
>
> But as Greg says TYPEDEFS ARE EVIL.
Yes, they are. Buuuuuuut, you _can_ make the code around them a little
less evil. If you _must_ use a typedef, you could do something like
this:
#define XEN_CAP_INFO_LEN_BYTES 1024
typedef char [XEN_CAP_INFO_LEN_BYTES] xen_capabilities_info_t;
That way, you can do your thing without using the typedef in the sysfs
code. The very best way to do it is to skip the typedef altogether, and
just explicitly create char[] arrays with the length macro when you need
them.
If you did this, and dynamically allocated the buffer, your cap_show
function could look something like this:
+/* xen capabilities info */
+static ssize_t xen_cap_show(struct kobject *kobj,
+ struct attribute *attr,
+ char *sysfs_buf)
+{
+ ssize_t result = 0;
+ char *info_buf;
+
+ /* +1 is to make room for the \n in the snprintf */
+ info_buf = kmalloc(XEN_CAP_INFO_LEN_BYTES+1, GFP_KERNEL);
+ if(!info_buf)
+ return result;
+
+ if (HYPERVISOR_xen_version(XENVER_capabilities, &info_buf))
+ goto out;
+
+ result = snprintf(sysfs_buf, XEN_CAP_INFO_LEN_BYTES+1,
+ "%s\n", info_buf);
+out:
+ kfree(info_buf);
+ return result;
+}
> Last resort would be to use the funky gcc #include_next to override
> the xen hcall headers with a linux-specific hcall headers. But I don't
> know if that would be cool with lkml either.
I've never seen that done, so I'd try to steer clear.
-- Dave
On 30 Jan 2006, at 17:38, Dave Hansen wrote:
> Yes, they are. Buuuuuuut, you _can_ make the code around them a little
> less evil. If you _must_ use a typedef, you could do something like
> this:
>
> #define XEN_CAP_INFO_LEN_BYTES 1024
> typedef char [XEN_CAP_INFO_LEN_BYTES] xen_capabilities_info_t;
Is that really better than just referencing the typedef? I've always
considered them okay for simple scalar and array types, even if they
are to be avoided for structure types. Is it the size aspect that is
the problem (e.g., a typedef'ed type should be okay to allocate on the
stack)?
-- Keir
On Mon, 2006-01-30 at 17:53 +0000, Keir Fraser wrote:
> On 30 Jan 2006, at 17:38, Dave Hansen wrote:
>
> > Yes, they are. Buuuuuuut, you _can_ make the code around them a little
> > less evil. If you _must_ use a typedef, you could do something like
> > this:
> >
> > #define XEN_CAP_INFO_LEN_BYTES 1024
> > typedef char [XEN_CAP_INFO_LEN_BYTES] xen_capabilities_info_t;
>
> Is that really better than just referencing the typedef? I've always
> considered them okay for simple scalar and array types, even if they
> are to be avoided for structure types.
One reason they're "evil" is that they hide what is going on. It is
worse for structures, but doing it for arrays still hides what is there,
and a hapless programmer can easily jump off the end of the stack
without realizing it. I think the kernel style is to be as explicit as
possible, especially when it isn't too verbose.
In this case, I expect a programmer declaring a 'char foo[XEN_FOO]'
array on the stack to be much more likely to go look up how big XEN_FOO
is than one who sees a 'xen_capabilities_info_t foo'.
> Is it the size aspect that is
> the problem (e.g., a typedef'ed type should be okay to allocate on the
> stack)?
The size is the issue. A typedef just makes it a little bit harder to
track down. That's why typedefs are evil. ;)
-- Dave
On Mon, Jan 30, 2006 at 05:53:30PM +0000, Keir Fraser wrote:
>
> On 30 Jan 2006, at 17:38, Dave Hansen wrote:
>
> >Yes, they are. Buuuuuuut, you _can_ make the code around them a little
> >less evil. If you _must_ use a typedef, you could do something like
> >this:
> >
> >#define XEN_CAP_INFO_LEN_BYTES 1024
> >typedef char [XEN_CAP_INFO_LEN_BYTES] xen_capabilities_info_t;
>
> Is that really better than just referencing the typedef? I've always
> considered them okay for simple scalar and array types, even if they
> are to be avoided for structure types. Is it the size aspect that is
> the problem (e.g., a typedef'ed type should be okay to allocate on the
> stack)?
No, a typedef for a simple array in the kernel is just foolish. Please
read the archives and my old OLS paper on coding style as to why
typedefs are not acceptable in the kernel, except for a very few core
types (and you should not be creating those types of objects...)
thanks,
greg k-h