lkml.org 
[lkml]   [2011]   [Jan]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH V8 09/13] posix clocks: introduce dynamic clocks
    Date
    On Friday 31 December 2010, Richard Cochran wrote:
    > This patch adds support for adding and removing posix clocks. The
    > clock lifetime cycle is patterned after usb devices. Each clock is
    > represented by a standard character device. In addition, the driver
    > may optionally implemented custom character device operations.
    >
    > The posix clock and timer system calls listed below now work with
    > dynamic posix clocks, as well as the traditional static clocks.
    > For the performance critical calls (eg clock_gettime) the code path
    > from the traditional static clocks is preserved.

    Combining the operations structures and using container_of as you did
    looks much better than before, but I had something slightly different
    in mind:

    The way that other subsystems do this is to pass a pointer to the
    actual low-level object (struct posix_clock in your case) to the
    abstracted functions, while you pass a pointer to the operations
    structure. This has the advantage of keeping the definition of
    posix_clock private to posix-clock.c, but it is something we do
    very rarely.

    I can see disadvantages with your approach: You still need to dynamically
    allocate the posix_clock in posix_clock_create(), and the operations
    structure cannot be const, which is a theoretical security problem
    if there is a hole that can replace one of the pointers with a
    reference to user memory.

    I would recommend changing this to the more common model, where you
    passs the (publically defined) struct posix_clock to all operations
    and can do something like:

    struct my_posix_clock {
    ...
    struct posix_clock pclk;
    } this_clock = {
    ...
    .pclk = {
    .ops = &my_posix_clock_operations,
    }
    };

    int my_init(void)
    {
    return posix_clock_operations_register(&my_posix_clock.pclk);
    }

    void my_exit(void)
    {
    posix_clock_operations_unregister(&my_posix_clock.pclk);
    }

    It should be just a trivial change and just affect how easy it is for
    other people to understand the code if they are already familiar
    with other kernel code.

    Overall, your series looks really good now, it would be nice if this
    could still make it into 2.6.38.

    Arnd



    \
     
     \ /
      Last update: 2011-01-07 15:13    [W:0.024 / U:30.304 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site