lkml.org 
[lkml]   [2009]   [Nov]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH, RFC] panic-note: Annotation from user space for panics
    From
    Date
    A pair of nit-picks.

    On Wed, 2009-11-11 at 21:13 -0500, David VomLehn wrote:
    > @@ -0,0 +1,293 @@
    > +/*
    > + * panic-note.c

    No need to type file name there.

    > + *
    > + * Allow a blob to be registered with the kernel that will be printed if
    > + * the kernel panics.
    > + *
    > + * Copyright (C) 2009 Cisco Systems, Inc.
    > + *
    > + * 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., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
    > + */
    > +
    > +/* Open issues:
    > + * Where should the panic_note file be created? It's almost like a sysctl,
    > + * but doesn't follow the same rules. When you write to a sysctl file, the
    > + * previous data is replaced. When you write to the panic_note file, you
    > + * can append to the end of the existing data.
    > + */

    Please, take a look at what is the kernel comment style at
    Documentation/CodingStyle. We use

    /*
    * Multi-line
    * comment
    */

    style.

    > +
    > +#include <linux/semaphore.h>
    > +#include <linux/fs.h>
    > +#include <linux/proc_fs.h>
    > +#include <linux/module.h>
    > +#include <linux/uaccess.h>
    > +
    > +/* Maximum size, in bytes, allowed for the blob. Having this limit prevents
    > + * an inadvertant denial of service attack that might happen if someone with
    > + * root privileges was automatically generating this note and the generator
    > + * had an infinite loop. Perhaps this is more of a a denial of service
    > + * suicide. */
    > +#define PANIC_NOTE_SIZE (PAGE_SIZE * 4)
    > +
    > +/*
    > + * struct panic_note_data - Information about the panic note
    > + * @n: Number of bytes in the note
    > + * @p: Pointer to the data in the note
    > + * @sem: Semaphore controlling access to data in the note
    > + */
    > +struct panic_note_state {
    > + size_t n;
    > + void *p;
    > + struct rw_semaphore sem;
    > +};
    > +
    > +static struct panic_note_state panic_note_state = {
    > + 0, NULL, __RWSEM_INITIALIZER(panic_note_state.sem)
    > +};
    > +static const struct file_operations panic_note_fops;
    > +static struct inode_operations panic_note_iops;
    > +static struct proc_dir_entry *panic_note_entry;
    > +
    > +/*
    > + * panic_note_print - display the panic note
    > + * @priority: Printk priority to use, e.g. KERN_EMERG
    > + */
    > +void panic_note_print()
    > +{
    > + int i;
    > + int linelen;
    int i, lineline is more compact.

    > +
    > + /* We skip the semaphore stuff because we're in a panic situation and
    > + * the scheduler isn't available in case the semaphore is already owned
    > + * by someone else */
    > + for (i = 0; i < panic_note_state.n; i += linelen) {
    > + const char *p;
    > + int remaining;
    > + const char *nl;

    const char *p, *nl is more compact. And there are several more places.
    But this is matter of taste, really.

    > +
    > + p = panic_note_state.p + i;
    > + remaining = panic_note_state.n - i;
    > +
    > + nl = memchr(p, '\n', remaining);
    > +
    > + if (nl == NULL) {
    > + linelen = remaining;
    > + pr_emerg("%.*s\n", linelen, p);
    > + } else {
    > + linelen = nl - p + 1;
    > + pr_emerg("%.*s", linelen, p);
    > + }
    > + }
    > +}
    > +
    > +/*
    > + * read_write_size - calculate the limited copy_to_user/copy_from_user count
    > + * @nbytes: The number of bytes requested
    > + * @pos: Offset, in bytes, into the file
    > + * @size: Maximum I/O offset, in bytes. For a read, this is the actual
    > + * number of bytes in the file, since you can't read past
    > + * the end. Writes can be done after the number of bytes in the
    > + * file, so this is the maximum possible file size, minus one.
    > + *
    > + * Returns the number of bytes to copy.
    > + */
    > +static ssize_t read_write_size(size_t nbytes, loff_t pos, size_t size)
    > +{
    > + ssize_t retval;
    > +
    > + if (pos >= size)
    > + retval = 0;
    > +
    Unnecessary \n
    > + else {
    > + retval = size - pos;
    > + if (retval > nbytes)
    > + retval = nbytes;
    > + }
    > +
    > + return retval;
    > +}


    --
    Best Regards,
    Artem Bityutskiy (Артём Битюцкий)

    --
    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: 2009-11-17 10:07    [W:0.063 / U:0.068 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site