lkml.org 
[lkml]   [2007]   [Feb]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 05/44 take 2] [UBI] internal common header
    On Tue, Feb 20, 2007 at 03:05:53PM +0200, Artem Bityutskiy wrote:
    > On Mon, 2007-02-19 at 10:54 +0000, Christoph Hellwig wrote:
    > > On Sat, Feb 17, 2007 at 06:54:49PM +0200, Artem Bityutskiy wrote:
    > > > +#ifndef __UBI_UBI_H__
    > > > +#define __UBI_UBI_H__
    > > > +
    > > > +#include <linux/mtd/ubi.h>
    > > > +
    > > > +/* Version of this UBI implementation */
    > > > +#define UBI_VERSION 1
    > > We shouldn't have versions for inkernel interfaces.
    >
    > What do you mean? It is internal version just for future help: if we
    > develop incompatible UBI2 the old UBI will reject the new images.

    In that case it's not an *implementation* version number, but rather
    an on-disk *format* version number. There's a difference. It's also
    often not used much, since another way of dealing with the problem is
    to mark major each on-disk version with a different magic number.
    (Many new filesystems these also will use an 8-byte magic string, such
    as "UBI-FS1\n" so that it's a bit users who look at an image using a
    hex editor to see what the heck it is, and also since with a longer
    magic string there is less likelihood of an accidental collision given
    that we don't have a central registry of magic numbers.)

    BTW, it's kind of silly to use an enum here:

    /*
    * Magic numbers of the UBI headers.
    *
    * @UBI_EC_HDR_MAGIC: erase counter header magic number (ASCII "UBI#")
    * @UBI_VID_HDR_MAGIC: volume identifier header magic number (ASCII "UBI!")
    */
    enum {
    UBI_EC_HDR_MAGIC = 0x55424923,
    UBI_VID_HDR_MAGIC = 0x55424921
    };

    Why isn't this being done via #define? It's not like this is any kind
    of an enumerated type, especially since it's being installed into a
    32bit type, and not even an enum type.

    Also while taking another look at your patches, note that that your
    mix of C99 types and your UBI magic types is a bad idea:

    struct ubi_ec_hdr {
    ubi32_t magic;
    uint8_t version;
    uint8_t padding1[3];
    ubi64_t ec; /* Warning: the current limit is 31-bit anyway! */
    ubi32_t vid_hdr_offset;
    ...

    It appears that the reason why you are doing this is because you think
    you need the (packed) attribute. Not needed; Linux assumes all over
    the place 16, 32, and 64 types are packed. If Linux is ever compiled
    on an architecture where this isn't true, the compiler will probably
    need to be fixed so these assumptions are true, since all manner of
    things will break.

    It would be much better to use __be32 and __be64, so you get better
    type checking, and you will catch bugs caused by forgetting to use
    be32_to_cpu, et. al.

    BTW, it might also be a good idea to run UBI through sparse to catch bugs.

    - Ted
    -
    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: 2007-02-20 15:59    [W:0.023 / U:59.612 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site