Date: Fri, 18 Dec 2015 18:13:03 -0700 From: Ian Lepore <ian@freebsd.org> To: John-Mark Gurney <jmg@funkthat.com>, Warner Losh <imp@bsdimp.com> Cc: freebsd-arm <freebsd-arm@freebsd.org>, Rui Paulo <rpaulo@me.com> Subject: Re: mount_smbfs Message-ID: <1450487583.25138.132.camel@freebsd.org> In-Reply-To: <20150123213619.GP1949@funkthat.com> References: <54B9DCD1.3040306@foxvalley.net> <4759EAA0-D4AA-4923-9350-B7E753819169@me.com> <6E32991C3BD8465DB8DB0E65DFDA47AA@ad.peach.ne.jp> <20150123195403.GO1949@funkthat.com> <ED004DCD-35B9-4F2E-9F5D-41BF33E66F01@bsdimp.com> <20150123213619.GP1949@funkthat.com>
index | next in thread | previous in thread | raw e-mail
[-- Attachment #1 --]
On Fri, 2015-01-23 at 13:36 -0800, John-Mark Gurney wrote:
> Warner Losh wrote this message on Fri, Jan 23, 2015 at 13:21 -0800:
> >
> > > On Jan 23, 2015, at 11:54 AM, John-Mark Gurney <jmg@funkthat.com> wrote:
> > >
> > > Daisuke Aoyama wrote this message on Sat, Jan 24, 2015 at 03:07 +0900:
> > > > Quick hack patch is attached.
> > >
> > > Please use {l,b}e16dec, or if the code is suppose to be native endian,
> > > make it dependant on __NO_STRICT_ALIGNMENT and add the proper endian
> > > swap, not __arm__ as there are other arches that require the same fix...
> >
> > If there???s just a couple of places that need this, don???t bother making them dependent
> > on __NO_STRICT_ALIGNMENT. That clutters things up a bit too much. Given the 3
> > or 4 places this is used, and the relative infrequency of the calls, just doing a memcpy
> > unconditionally is always correct and reduces the risk of one branch of the #if being
> > changed w/o the other. Since it is already using NBENCODE(), I think that using
> > {l,b}e16enc (not dec) would be a larger code churn.
>
> Clearly neither of us looked at the code closely... NBENCODE should be
> rewritten to take a pointer and use le16enc... Then memsetw should just
> call NBENCODE internally as it goes...
Well I looked at the code closely, even if it did take me almost a year
to get around to it. :)
The conclusion I reached is that alignment and endian problems should
just be turned into a non-issue by discarding the existing macro and
memsetw() function and writing a new inline function that deals purely
with bytes instead of 16-bit values.
It's up for review on phabricator at https://reviews.freebsd.org/D4622
And I'll also attach the diff to this mail for anyone who wants to test
it (which I can't do, no smb server).
-- Ian
[-- Attachment #2 --]
Index: contrib/smbfs/lib/smb/nb_name.c
===================================================================
--- contrib/smbfs/lib/smb/nb_name.c (revision 292290)
+++ contrib/smbfs/lib/smb/nb_name.c (working copy)
@@ -143,15 +143,13 @@ nb_encname_len(const char *str)
return len;
}
-#define NBENCODE(c) (htole16((u_short)(((u_char)(c) >> 4) | \
- (((u_char)(c) & 0xf) << 8)) + 0x4141))
+static inline void
+nb_char_encode(u_char **ptr, u_char c, int n)
+{
-static void
-memsetw(char *dst, int n, u_short word)
-{
while (n--) {
- *(u_short*)dst = word;
- dst += 2;
+ *(*ptr)++ = 0x41 + (c >> 4);
+ *(*ptr)++ = 0x41 + (c & 0x0f);
}
}
@@ -165,19 +163,15 @@ nb_name_encode(struct nb_name *np, u_char *dst)
*cp++ = NB_ENCNAMELEN;
name = np->nn_name;
if (name[0] == '*' && name[1] == 0) {
- *(u_short*)cp = NBENCODE('*');
- memsetw(cp + 2, NB_NAMELEN - 1, NBENCODE(' '));
- cp += NB_ENCNAMELEN;
+ nb_char_encode(&cp, '*', 1);
+ nb_char_encode(&cp, ' ', NB_NAMELEN - 1);
} else {
- for (i = 0; *name && i < NB_NAMELEN - 1; i++, cp += 2, name++)
- *(u_short*)cp = NBENCODE(toupper(*name));
- i = NB_NAMELEN - i - 1;
- if (i > 0) {
- memsetw(cp, i, NBENCODE(' '));
- cp += i * 2;
- }
- *(u_short*)cp = NBENCODE(np->nn_type);
- cp += 2;
+ for (i = 0; i < NB_NAMELEN - 1; i++)
+ if (*name != 0)
+ nb_char_encode(&cp, toupper(*name++), 1);
+ else
+ nb_char_encode(&cp, ' ', 1);
+ nb_char_encode(&cp, np->nn_type, 1);
}
*cp = 0;
if (np->nn_scope == NULL)
home |
help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1450487583.25138.132.camel>