Re: [Xen-devel] [PATCH 2.6.12.6-xen] sysfs attributes for xen

[Anthony Liguori <aliguori@us.ibm.com>]

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