[PATCH v5] New VFS module vfs_fruit

[Jeremy Allison]

On Mon, Jul 28, 2014 at 11:17:49AM +0200, Ralph Böhme wrote:
> On Mon, Jul 28, 2014 at 11:10:43AM +0200, Ralph Böhme wrote:
> > here's the updated patchset.
> 
> not there, but here! :)

Home stretch now, honest :-).

A few minor comments:

----------------------------------------------------------
commit bad69d1cd6e94942beac765832a1da23464f10fd
Author: Ralph Boehme <rb at sernet.de>
Date:   Mon Jun 23 16:59:45 2014 +0200

    New VFS module vfs_fruit

+struct ad_entry {
+       off_t     ade_off;
+       ssize_t   ade_len;
+};

JRA: Can these values ever be negative ?
Shouldn't they be unsigned values ?

+static int ad_getdate(const struct adouble *ad,
+                     unsigned int dateoff,
+                     uint32_t *date)
+{
+       int xlate = (dateoff & AD_DATE_UNIX);

+static int ad_setdate(struct adouble *ad, unsigned int dateoff, uint32_t date)
+{
+       int xlate = (dateoff & AD_DATE_UNIX);

JRA: Shouldn't xlate be a bool not an int ?

+static bool ad_pack(struct adouble *ad)
+{
+       uint32_t       eid;
+       uint16_t       nent;
+       ssize_t        bufsize;
+       off_t          offset = 0;

JRA: off_t is a *signed* datatype.

+       bufsize = talloc_get_size(ad->ad_data);

JRA: bufsize should be unsigned here, not ssize_t.

As you're using offset via RSIVAL, I think
it might be safer & easier to use:

+       uint32_t        bufsize;
+       uint32_t        offset = 0;

as that way we're contraining the data ranges
more sanely. In fact you're already doing
that with off in ad_unpack() following.

+static int ad_convert(struct adouble *ad, int fd)
+{
+       int rc = 0;
+       char *map = MAP_FAILED;
+       ssize_t origlen;
+
+       origlen = ad_getentryoff(ad, ADEID_RFORK) +
+               ad_getentrylen(ad, ADEID_RFORK);

JRA: Again, I think origlen should be size_t
(unsigned) here.

----------------------------------------------------------
commit 075870eeee8320b3dc3bbabc0f5e31e6f2d8b751
Author: Ralph Boehme <rb at sernet.de>
Date:   Mon Jun 23 17:01:30 2014 +0200

    vfs_fruit: add manpage

JRA: Fix 3 spelling mistakes :

	sould -> should
	degredations -> degradations
	charcters -> characters

----------------------------------------------------------
commit 54968e0ea8d04b2cbfd885c87dde50c628332f8f
Author: Ralph Boehme <rb at sernet.de>
Date:   Thu Jul 10 16:32:15 2014 +0200

    Fix AFP_BackupTime byte order

    AFP_BackupTime value must be 0x80000000 and all existing defines use
    native byte order, not byte swapped.

JRA: Can you squash this into commit bad69d1cd6e94942beac765832a1da23464f10fd
please ? Checking in the pristine module is good enough, we
don't really need to see how the sausage got made :-).
----------------------------------------------------------
commit 7111376b634a0ef9c30b914ac45b0ad77f0f6c85
Author: Ralph Boehme <rb at sernet.de>
Date:   Thu Jul 10 16:36:25 2014 +0200

    s4:torture:vfs_fruit: add uintxx defines

    In oder to be able to include MacExtensions.h we need uint32 type and
    there doesn't seem to be a way includes can pick up
    source3/include/includes.h where the types are defined.

JRA: Can't this commit change the usage of uint32
inside MacExtensions.h to the standard uint32_t and
use that instead ?

Hope I'm not being too picky, shout at me if I'm
driving you nuts :-).

Cheers,

	Jeremy.