[PATCH v5] New VFS module vfs_fruit

Ralph Böhme rb at sernet.de
Wed Aug 6 07:25:31 MDT 2014


Hi Jeremy,

thanks for reviewing, comments below.

On Tue, Aug 05, 2014 at 03:36:14PM -0700, Jeremy Allison wrote:
> 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 ?

Probably makes sense to use size_t instead, so I've changed both to
size_t in the next patch series.

> +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 ?

Yep. Updated int teh next patch series.

> +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.

Done.

> +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.

Done.

> ----------------------------------------------------------
> 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

Done.

> ----------------------------------------------------------
> 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 ?

Huh, there's no such commit in this patch series nor in my git bin.


> 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 ?

Good idea. I've split this commit and squashed it with the "Fix
AFP_BackupTime byte order" commit.

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

Not the least!

Updated patchset coming in a minute.

Thanks!
-Ralph

-- 
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de,mailto:kontakt@sernet.de


More information about the samba-technical mailing list