lkml.org 
[lkml]   [2006]   [Jan]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    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    [W:0.037 / U:90.388 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site