Date: Wed, 13 Apr 2011 17:51:21 +0200 From: Pawel Jakub Dawidek <pjd@FreeBSD.org> To: Adrian Chadd <adrian@FreeBSD.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r220559 - in head/sys: conf geom Message-ID: <20110413155121.GU40382@garage.freebsd.pl> In-Reply-To: <201104120810.p3C8AQs0014704@svn.freebsd.org> References: <201104120810.p3C8AQs0014704@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--gSSGYPGSs0dvYOj7 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Apr 12, 2011 at 08:10:26AM +0000, Adrian Chadd wrote: > Author: adrian > Date: Tue Apr 12 08:10:25 2011 > New Revision: 220559 > URL: http://svn.freebsd.org/changeset/base/220559 >=20 > Log: > Introduce geom_map, a GEOM provider designed for use by > embedded flash stores. > =20 > Some devices - notably those with uboot - don't have an > explicit partition table (eg like Redboot's FIS.) > geom_map thus provides an easy way to export the hard-coded > flash layout as geom providers for use by filesystems and > other tools. > =20 > It also includes a "search" function which allows for > dynamic creation of partition layouts where the device only > has a single hard-coded partition. For example, if > there is a "kernel+rootfs" partition, a single image can > be created which appends the rootfs after the kernel with > an appropriate search string. geom_map can be told to > search for said search string and create a partition > beginning after it. > =20 > Submitted by: Aleksandr Rybalko <ray@dlink.ua> I'm sorry for not finding time to review this change before your commit, but I do have some comments. Before I get to the code, I'd like to state that I fully agree with Andrew Thompson that having 4GB limit is really wrong. We had this bigdisk project for a long time trying to remove all such old assumptions about disk sizes and committing a GEOM class that have a limit of 4GB for media size is really wrong direction. We should really treat this as a policy for new GEOM classes. Changing GEOM_MAP to work with 64bit offsets is not a big change and I'd really like to see it being committed. Maybe Aleksandr (CCed) would like to work on this? > +#include <sys/param.h> > +#include <sys/bus.h> > +#include <sys/errno.h> > +#include <sys/endian.h> > +#include <sys/systm.h> > +#include <sys/kernel.h> > +#include <sys/fcntl.h> > +#include <sys/malloc.h> > +#include <sys/bio.h> > +#include <sys/lock.h> > +#include <sys/mutex.h> > + > +#include <sys/sbuf.h> > +#include <geom/geom.h> > +#include <geom/geom_slice.h> I'd move empty line from above sys/sbuf.h to below it. > +#define MAP_CLASS_NAME "MAP" Spaces here should be replaced with tabs. > +struct map_desc { > + uint8_t name [16]; /* null-terminated name */ I'd remove white-spaces between array name and size. Also in other places. > +static int > +g_map_ioctl(struct g_provider *pp, u_long cmd, void *data, int fflag, st= ruct thread *td) > +{ > + return (ENOIOCTL); > +} This can be removed. If the ioctl method is not specified, GEOM will return ENOIOCTL for you. > +static int > +g_map_access(struct g_provider *pp, int dread, int dwrite, int dexcl) > +{ > + struct g_geom *gp =3D pp->geom; Two spaces instead of one. You can also think about moving initializations below declarations. > + struct g_slicer *gsp =3D gp->softc; > + struct g_map_softc *sc =3D gsp->softc; > +static int > +g_map_start(struct bio *bp) > +{ > + struct g_provider *pp; > + struct g_geom *gp; Two spaces instead of one. > + struct g_map_softc *sc; > + struct g_slicer *gsp; > + int idx; Tabs instead of single space. > + pp =3D bp->bio_to; > + idx =3D pp->index; > + gp =3D pp->geom; > + gsp =3D gp->softc; > + sc =3D gsp->softc; > + if (bp->bio_cmd =3D=3D BIO_GETATTR) { > + if (g_handleattr_int(bp, MAP_CLASS_NAME "::entry", > + sc->entry[idx])) > + return (1); > + if (g_handleattr_int(bp, MAP_CLASS_NAME "::dsize", > + sc->dsize[idx])) > + return (1); This should look like this: if (g_handleattr_int(bp, MAP_CLASS_NAME "::entry", sc->entry[idx])) { return (1); } if (g_handleattr_int(bp, MAP_CLASS_NAME "::dsize", sc->dsize[idx])) { return (1); } > +static void > +g_map_dumpconf(struct sbuf *sb, const char *indent, struct g_geom *gp, > + struct g_consumer *cp __unused, struct g_provider *pp) The line should be started with four spaces. > + if (indent =3D=3D NULL) { > + sbuf_printf(sb, " entry %d", sc->entry[pp->index]); > + sbuf_printf(sb, " dsize %d", sc->dsize[pp->index]); > + } else { > + sbuf_printf(sb, "%s<entry>%d</entry>\n", indent, > + sc->entry[pp->index]); > + sbuf_printf(sb, "%s<dsize>%d</dsize>\n", indent, > + sc->dsize[pp->index]); > + } Line should be continued using four spaces. > +#include <sys/ctype.h> This shouldn't be here. > +static struct g_geom * > +g_map_taste(struct g_class *mp, struct g_provider *pp, int insist) > +{ > + struct g_geom *gp; > + struct g_consumer *cp; > + struct g_map_softc *sc; > + int error , sectorsize, i, ret; > + struct map_desc *head; > + u_int32_t start =3D 0, end =3D 0, size =3D 0, off, readonly; > + const char *name; > + const char *at; > + const char *search; > + int search_start =3D 0, search_end =3D 0; > + u_char *buf; > + uint32_t offmask; > + u_int blksize;/* NB: flash block size stored as stripesize */ > + off_t offset; White-spaces need cleanups. Three 'const char *' should use single line. uint32_t is preferred over u_int32_t and only one of them should be used for consistency. > + g_trace(G_T_TOPOLOGY, "map_taste(%s,%s)", mp->name, pp->name); > + g_topology_assert(); > + if (!strcmp(pp->geom->class->name, MAP_CLASS_NAME)) strcmp() doesn't return bool, it returns int so should be compared to 0. > + gp =3D g_slice_new(mp, MAP_MAXSLICE, pp, &cp, &sc, sizeof(*sc), > + g_map_start); Four spaces. > + if (powerof2(cp->provider->mediasize)) > + offmask =3D cp->provider->mediasize - 1; > + else > + offmask =3D 0xffffffff; /* XXX */ > + > + g_topology_unlock(); > + head =3D NULL; > + offset =3D cp->provider->mediasize - blksize; > + g_topology_lock(); Why do you drop the topology lock here? Seems wrong. > + for (i =3D 0; i < MAP_MAXSLICE; i++) { > + search_start =3D search_end =3D start =3D end =3D off =3D readonly =3D= 0; > + > + ret =3D resource_string_value("map", i, "at", &at); > + if (ret) > + continue; 'ret' is not bool. > + > + /* Check if my provider */ > + if (strncmp(pp->name, at, strlen(at))) > + continue; strncmp() is not bool. > + ret =3D resource_string_value("map", i, "start", &search); > + > + if (!ret && strncmp(search, "search", 6) =3D=3D 0) { > + uint32_t search_offset, search_start =3D 0; > + uint32_t search_step =3D 0; > + const char *search_key; > + char key[255]; > + int c; > + > + ret =3D resource_int_value("map", i, "searchstart", > + &search_start); > + ret =3D resource_int_value("map", i, "searchstep", > + &search_step); Four spaces. > + printf("GEOM_MAP: searchkey=3D\"%s\"\n", search_key); Is this a leftover debug printf? > + for (search_offset =3D search_start; > + search_offset < cp->provider->mediasize && start =3D=3D 0; > + search_offset +=3D search_step) { > + buf =3D g_read_data(cp,=20 > + rounddown(search_offset, sectorsize),=20 > + roundup(strlen(search_key), sectorsize),=20 > + NULL); Four spaces. It is better to drop the topology lock when doing g_read_data(). > + if (buf !=3D NULL && strncmp( > + buf + search_offset % sectorsize,=20 > + key, strlen(search_key)) =3D=3D 0) > + start =3D search_offset; This is good example why do we use four spaces to continue previous line. In the above code it is hard to see where the diff ends and where the code begins. > + ret =3D resource_int_value("map", i, "searchstart", &search_start); > + ret =3D resource_int_value("map", i, "searchstep", &search_step); > + if (ret) > + search_step =3D 0x10000U; First 'ret' is not checked. > + buf =3D g_read_data(cp,=20 > + rounddown(search_offset, sectorsize),=20 > + roundup(strlen(search_key), sectorsize),=20 > + NULL); Again, it is better to drop the topology lock. > + ret =3D resource_int_value("map", i, "offset", &off); > + ret =3D resource_int_value("map", i, "readonly", &readonly); > + ret =3D resource_string_value("map", i, "name", &name); > + /* No name or error read name */ > + if (ret) > + continue; First and second 'ret' is not checked. > + if (off > size) > + printf("%s: off(%d) > size(%d) for \"%s\"\n",=20 > + __func__, off, size, name); Is this a leftover debug printf? > + error =3D g_slice_config(gp, i, G_SLICE_CONFIG_SET, start + off,=20 > + size - off, sectorsize, "map/%s", name); Four spaces. > + printf("MAP: %08x-%08x, offset=3D%08x \"map/%s\"\n", > + (uint32_t) start, > + (uint32_t) size, > + (uint32_t) off, > + name > + ); Four spaces. Not need to separate cast and variable with space. > + if (error) > + printf("%s g_slice_config returns %d for \"%s\"\n",=20 > + __func__, error, name); Leftover debug printf? > + sc->entry[i] =3D off; > + sc->dsize[i] =3D size - off; > + sc->readonly[i] =3D readonly ? 1 : 0; > + } > +=09 > + > + if (i =3D=3D 0) > + return (NULL); Redundant empty line before 'if'. > +static void > +g_map_config(struct gctl_req *req, struct g_class *mp, const char *verb) > +{ > + struct g_geom *gp; > + > + g_topology_assert(); > + gp =3D gctl_get_geom(req, mp, "geom"); > + if (gp =3D=3D NULL) > + return; > + gctl_error(req, "Unknown verb"); > +} Seems to be a no-op. GEOM can cope if this method is not specified. --=20 Pawel Jakub Dawidek http://www.wheelsystems.com FreeBSD committer http://www.FreeBSD.org Am I Evil? Yes, I Am! http://yomoli.com --gSSGYPGSs0dvYOj7 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.14 (FreeBSD) iEYEARECAAYFAk2lxngACgkQForvXbEpPzSgKACeM/fQzuiH5yafKOuMMqOEojqP rqsAoNGr3V9zQLddb7/U2UIcjlqqHsxz =ZpLd -----END PGP SIGNATURE----- --gSSGYPGSs0dvYOj7--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20110413155121.GU40382>