On Sun, Dec 25, 2011 at 11:42:49PM +0800, Liu Yuan wrote: > From: Liu Yuan <tailai.ly at taobao.com> > > All the objects(snap, trunk, data) in the farm is based on the > operations of the sha1_file. > > sha1_file provide us some useful features: > > - Regardless of object type, all objects are all in deflated with zlib, > and have a header that not only specifies their tag, but also size > information about the data in the object. It's worth noting that the > SHA1 hash that is used to name the object is always the hash of this > _compressed_ object, not the original data. why? > > - the general consistency of an object can always be tested independently > of the contents or the type of the object: all objects can be validated > by verifying that > (a) their hashes match the content of the file and > (b) the object successfully inflates to a stream of bytes that > forms a sequence of <sha1_file_hdr> + <binary object data> These comments should not only be in the commit message but also in the source code. > --- /dev/null > +++ b/sheep/farm.h Any reason the header is not inside the farm/ subdirectory? > + return farm_obj_dir; > +} > + > +static void fill_sha1_path(char *pathbuf, const unsigned char *sha1) > +{ > + int i; > + for (i = 0; i < SHA1_LEN; i++) { > + static char hex[] = "0123456789abcdef"; I'd rather have a define for the hex chars, and defined SHA1_LEN as strlen(HEX_CHARS) which any recent compiler should optimize to a constant expression. > + objdir = get_object_directory(); > + len = strlen(objdir); > + > + /* '/' + sha1(2) + '/' + sha1(38) + '\0' */ > + if (len + 43 > PATH_MAX) > + panic("insanely long object directory %s", objdir); I'd rather check the path length during startup and refuse to start sheep instead of aborting later on. > + memcpy(buf, objdir, len); > + buf[len] = '/'; > + buf[len+3] = '/'; > + buf[len+42] = '\0'; > + fill_sha1_path(buf + len + 1, sha1); Can't we use the strbuf helpers here to make the code readable and less fragile? > + setxattr(name, CNAME, &count, CSIZE, 0); error handling? > + void *map; > + > + if (fd < 0) { > + perror(filename); > + return NULL; > + } > + if (fstat(fd, &st) < 0) { > + close(fd); > + return NULL; > + } > + map = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0); > + close(fd); > + if (-1 == (int)(long)map) just use a if (map == MAP_FAILED) here. |