[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