Re: [patch] libssl/src/ssl/ssl_rsa.c
On Thu, May 08, 2014 at 22:39, Theo de Raadt wrote: > Your diff does not solve a problem. Specifically, I don't think it's worth changing such code back or forth either way, but using an extra variable to store the return value from a function can make debugging easier. I may want to look at the return value without having to step into the function.
Re: [patch] libssl/src/ssl/ssl_rsa.c
Your diff does not solve a problem.
> In case this is considered important enough...
> Remove unused "ret" from SSL_use_PrivateKey().
>
> - Michael
>
>
> Index: ssl_rsa.c
> ===
> RCS file: /cvs/src/lib/libssl/src/ssl/ssl_rsa.c,v
> retrieving revision 1.11
> diff -u -r1.11 ssl_rsa.c
> --- ssl_rsa.c 17 Apr 2014 21:37:37 - 1.11
> +++ ssl_rsa.c 9 May 2014 03:46:58 -
> @@ -273,8 +273,6 @@
> int
> SSL_use_PrivateKey(SSL *ssl, EVP_PKEY *pkey)
> {
> - int ret;
> -
> if (pkey == NULL) {
> SSLerr(SSL_F_SSL_USE_PRIVATEKEY, ERR_R_PASSED_NULL_PARAMETER);
> return (0);
> @@ -283,8 +281,7 @@
> SSLerr(SSL_F_SSL_USE_PRIVATEKEY, ERR_R_MALLOC_FAILURE);
> return (0);
> }
> - ret = ssl_set_pkey(ssl->cert, pkey);
> - return (ret);
> + return (ssl_set_pkey(ssl->cert, pkey));
> }
>
> #ifndef OPENSSL_NO_STDIO
>
[patch] libssl/src/ssl/ssl_rsa.c
Hi tech@,
In case this is considered important enough...
Remove unused "ret" from SSL_use_PrivateKey().
- Michael
Index: ssl_rsa.c
===
RCS file: /cvs/src/lib/libssl/src/ssl/ssl_rsa.c,v
retrieving revision 1.11
diff -u -r1.11 ssl_rsa.c
--- ssl_rsa.c 17 Apr 2014 21:37:37 - 1.11
+++ ssl_rsa.c 9 May 2014 03:46:58 -
@@ -273,8 +273,6 @@
int
SSL_use_PrivateKey(SSL *ssl, EVP_PKEY *pkey)
{
- int ret;
-
if (pkey == NULL) {
SSLerr(SSL_F_SSL_USE_PRIVATEKEY, ERR_R_PASSED_NULL_PARAMETER);
return (0);
@@ -283,8 +281,7 @@
SSLerr(SSL_F_SSL_USE_PRIVATEKEY, ERR_R_MALLOC_FAILURE);
return (0);
}
- ret = ssl_set_pkey(ssl->cert, pkey);
- return (ret);
+ return (ssl_set_pkey(ssl->cert, pkey));
}
#ifndef OPENSSL_NO_STDIO
Re: amd64 support for AR9485
On 2014/05/08 23:43, Sébastien Morand wrote:
> >
> >
> > You could try adding the ID to the athn driver to get it to match:
> >
> > Index: if_athn_pci.c
> > ===
> > RCS file: /cvs/src/sys/dev/pci/if_athn_pci.c,v
> > retrieving revision 1.14
> > diff -u -p -r1.14 if_athn_pci.c
> > --- if_athn_pci.c 6 Dec 2013 21:03:03 - 1.14
> > +++ if_athn_pci.c 8 May 2014 23:07:17 -
> > @@ -98,7 +98,8 @@ static const struct pci_matchid athn_pci
> > { PCI_VENDOR_ATHEROS, PCI_PRODUCT_ATHEROS_AR2427 },
> > { PCI_VENDOR_ATHEROS, PCI_PRODUCT_ATHEROS_AR9227 },
> > { PCI_VENDOR_ATHEROS, PCI_PRODUCT_ATHEROS_AR9287 },
> > - { PCI_VENDOR_ATHEROS, PCI_PRODUCT_ATHEROS_AR9300 }
> > + { PCI_VENDOR_ATHEROS, PCI_PRODUCT_ATHEROS_AR9300 },
> > + { PCI_VENDOR_ATHEROS, PCI_PRODUCT_ATHEROS_AR9485 }
> > };
> >
> > int
> >
> > No guarantees as to whether this is sufficient, or if further
> > work is needed, but it will at least get the device picked up by
> > the athn driver.
> >
>
> I did and It's doing what expected: card is detected but ifconfig athn0
> scan does not give me any result and it should since I'm far from one meter
> of the AP.
>
> Direct configuration give nothing either:
> $ ifconfig athn0 newid LSA wpapsk '***'
> ifconfig: newid: bad value
This should be "nwid", not newid.
I can't speak for athn, but my 6205 iwn is totally broken for scan,
but works fine if I can supply an nwid, so scan is not a requirement
for a useful adapter.
(Of course there may be other issues with the driver for your adapter,
but it's worth trying a bit more yourself).
> $ dmesg|grep athn0
> athn0 at pci2 dev 0 function 0 "Atheros AR9485" rev 0x01: apic 2 int 16
> athn0: AR9485 rev 1 (1T1R), ROM rev 0, address 18:67:b0:8f:d1:58
> athn0: could not initialize calibration
> athn0: could not initialize calibration
> athn0: unable to reset hardware; reset status 60
> athn0 at pci2 dev 0 function 0 "Atheros AR9485" rev 0x01: apic 2 int 16
> athn0: AR9485 rev 1 (1T1R), ROM rev 0, address 18:67:b0:8f:d1:58
> athn0: could not initialize calibration
>
> So I'm stuck with finding old usb hardware or waiting for a kind developer
> to have time and will to get my card supported ;-)
>
> Thanks for your help,
> Sébastien
Re: amd64 support for AR9485
>
>
> You could try adding the ID to the athn driver to get it to match:
>
> Index: if_athn_pci.c
> ===
> RCS file: /cvs/src/sys/dev/pci/if_athn_pci.c,v
> retrieving revision 1.14
> diff -u -p -r1.14 if_athn_pci.c
> --- if_athn_pci.c 6 Dec 2013 21:03:03 - 1.14
> +++ if_athn_pci.c 8 May 2014 23:07:17 -
> @@ -98,7 +98,8 @@ static const struct pci_matchid athn_pci
> { PCI_VENDOR_ATHEROS, PCI_PRODUCT_ATHEROS_AR2427 },
> { PCI_VENDOR_ATHEROS, PCI_PRODUCT_ATHEROS_AR9227 },
> { PCI_VENDOR_ATHEROS, PCI_PRODUCT_ATHEROS_AR9287 },
> - { PCI_VENDOR_ATHEROS, PCI_PRODUCT_ATHEROS_AR9300 }
> + { PCI_VENDOR_ATHEROS, PCI_PRODUCT_ATHEROS_AR9300 },
> + { PCI_VENDOR_ATHEROS, PCI_PRODUCT_ATHEROS_AR9485 }
> };
>
> int
>
> No guarantees as to whether this is sufficient, or if further
> work is needed, but it will at least get the device picked up by
> the athn driver.
>
I did and It's doing what expected: card is detected but ifconfig athn0
scan does not give me any result and it should since I'm far from one meter
of the AP.
Direct configuration give nothing either:
$ ifconfig athn0 newid LSA wpapsk '***'
ifconfig: newid: bad value
$ dmesg|grep athn0
athn0 at pci2 dev 0 function 0 "Atheros AR9485" rev 0x01: apic 2 int 16
athn0: AR9485 rev 1 (1T1R), ROM rev 0, address 18:67:b0:8f:d1:58
athn0: could not initialize calibration
athn0: could not initialize calibration
athn0: unable to reset hardware; reset status 60
athn0 at pci2 dev 0 function 0 "Atheros AR9485" rev 0x01: apic 2 int 16
athn0: AR9485 rev 1 (1T1R), ROM rev 0, address 18:67:b0:8f:d1:58
athn0: could not initialize calibration
So I'm stuck with finding old usb hardware or waiting for a kind developer
to have time and will to get my card supported ;-)
Thanks for your help,
Sébastien
Re: amd64 support for AR9485
On 2014/05/08 22:57, Sébastien Morand wrote:
> Hi,
>
> My wireless card AR9485 is not recognized by kernel (5.5). I'm having the
> error following error message:
> "Atheros AR9485" rev 0x01 at pci2 dev 0 function 0 not configured
>
> As far as I understand it means it's not supported but I can find a 2012
> email on tech list mentionning the ar9485 device in athn driver.
>
> I find out in athnreg.h:
> #define AR_SREV_VERSION_94850x240
> #define AR_SREV_REVISION_9485_101
>
> So it looks like it should be supported?
>
> pcidump -v give me:
> 2:0:0: Atheros AR9485
> 0x: Vendor ID: 168c Product ID: 0032
> 0x0004: Command: 0007 Status: 0010
> 0x0008: Class: 02 Subclass: 80 Interface: 00 Revision: 01
> 0x000c: BIST: 00 Header Type: 00 Latency Timer: 00 Cache Line Size: 10
> 0x0010: BAR mem 64bit addr: 0xf150/0x0008
> 0x0018: BAR empty ()
> 0x001c: BAR empty ()
> 0x0020: BAR empty ()
> 0x0024: BAR empty ()
> 0x0028: Cardbus CIS:
> 0x002c: Subsystem Vendor ID: 144d Product ID: 4105
> 0x0030: Expansion ROM Base Address:
> 0x0038:
> 0x003c: Interrupt Pin: 01 Line: 0b Min Gnt: 00 Max Lat: 00
> 0x0040: Capability 0x01: Power Management
> 0x0050: Capability 0x05: Message Signaled Interrupts (MSI)
> 0x0070: Capability 0x10: PCI Express
> Link Speed: 2.5 / 2.5 GT/s Link Width: x1 / x1
>
> pcidump -x give me:
> 2:0:0: Atheros AR9485
> 0x: 0032168c 0017 0281 0010
> 0x0010: f154
> 0x0020: 4105144d
> 0x0030: 0040 010b
>
> if any body can help me get it works, thanks (actually I try to play a
> little bit with the value in athnreg.h but it's not working :-) I'm not
> really a kernel developer so obviously I miss something.
>
> Regards,
> Sebastien
You could try adding the ID to the athn driver to get it to match:
Index: if_athn_pci.c
===
RCS file: /cvs/src/sys/dev/pci/if_athn_pci.c,v
retrieving revision 1.14
diff -u -p -r1.14 if_athn_pci.c
--- if_athn_pci.c 6 Dec 2013 21:03:03 - 1.14
+++ if_athn_pci.c 8 May 2014 23:07:17 -
@@ -98,7 +98,8 @@ static const struct pci_matchid athn_pci
{ PCI_VENDOR_ATHEROS, PCI_PRODUCT_ATHEROS_AR2427 },
{ PCI_VENDOR_ATHEROS, PCI_PRODUCT_ATHEROS_AR9227 },
{ PCI_VENDOR_ATHEROS, PCI_PRODUCT_ATHEROS_AR9287 },
- { PCI_VENDOR_ATHEROS, PCI_PRODUCT_ATHEROS_AR9300 }
+ { PCI_VENDOR_ATHEROS, PCI_PRODUCT_ATHEROS_AR9300 },
+ { PCI_VENDOR_ATHEROS, PCI_PRODUCT_ATHEROS_AR9485 }
};
int
No guarantees as to whether this is sufficient, or if further
work is needed, but it will at least get the device picked up by
the athn driver.
amd64 support for AR9485
Hi, My wireless card AR9485 is not recognized by kernel (5.5). I'm having the error following error message: "Atheros AR9485" rev 0x01 at pci2 dev 0 function 0 not configured As far as I understand it means it's not supported but I can find a 2012 email on tech list mentionning the ar9485 device in athn driver. I find out in athnreg.h: #define AR_SREV_VERSION_94850x240 #define AR_SREV_REVISION_9485_101 So it looks like it should be supported? pcidump -v give me: 2:0:0: Atheros AR9485 0x: Vendor ID: 168c Product ID: 0032 0x0004: Command: 0007 Status: 0010 0x0008: Class: 02 Subclass: 80 Interface: 00 Revision: 01 0x000c: BIST: 00 Header Type: 00 Latency Timer: 00 Cache Line Size: 10 0x0010: BAR mem 64bit addr: 0xf150/0x0008 0x0018: BAR empty () 0x001c: BAR empty () 0x0020: BAR empty () 0x0024: BAR empty () 0x0028: Cardbus CIS: 0x002c: Subsystem Vendor ID: 144d Product ID: 4105 0x0030: Expansion ROM Base Address: 0x0038: 0x003c: Interrupt Pin: 01 Line: 0b Min Gnt: 00 Max Lat: 00 0x0040: Capability 0x01: Power Management 0x0050: Capability 0x05: Message Signaled Interrupts (MSI) 0x0070: Capability 0x10: PCI Express Link Speed: 2.5 / 2.5 GT/s Link Width: x1 / x1 pcidump -x give me: 2:0:0: Atheros AR9485 0x: 0032168c 0017 0281 0010 0x0010: f154 0x0020: 4105144d 0x0030: 0040 010b if any body can help me get it works, thanks (actually I try to play a little bit with the value in athnreg.h but it's not working :-) I'm not really a kernel developer so obviously I miss something. Regards, Sebastien
[PATCH] rcs: no way to go, after usage was called
Hi tech,
there is no way you can go, after usage() was called, so dont't do it.
fritjof
Index: ci.c
===
RCS file: /cvs/src/usr.bin/rcs/ci.c,v
retrieving revision 1.216
diff -u -p -r1.216 ci.c
--- ci.c27 Oct 2013 18:31:24 - 1.216
+++ ci.c8 May 2014 19:46:13 -
@@ -97,6 +97,8 @@ checkin_usage(void)
" [-j[rev]] [-k[rev]] [-l[rev]] [-M[rev]] [-mmsg]\n"
" [-Nsymbol] [-nsymbol] [-r[rev]] [-sstate] [-t[str]]\n"
" [-u[rev]] [-wusername] [-xsuffixes] [-ztz] file ...\n");
+
+ exit(1);
}
/*
@@ -221,7 +223,6 @@ checkin_main(int argc, char **argv)
break;
default:
(usage)();
- exit(1);
}
}
@@ -231,7 +232,6 @@ checkin_main(int argc, char **argv)
if (argc == 0) {
warnx("no input file");
(usage)();
- exit(1);
}
if ((pb.username = getlogin()) == NULL)
Index: co.c
===
RCS file: /cvs/src/usr.bin/rcs/co.c,v
retrieving revision 1.117
diff -u -p -r1.117 co.c
--- co.c16 Apr 2013 20:24:45 - 1.117
+++ co.c8 May 2014 19:57:22 -
@@ -79,7 +79,6 @@ checkout_main(int argc, char **argv)
if (RCS_KWEXP_INVAL(kflag)) {
warnx("invalid RCS keyword substitution mode");
(usage)();
- exit(1);
}
break;
case 'l':
@@ -141,7 +140,6 @@ checkout_main(int argc, char **argv)
break;
default:
(usage)();
- exit(1);
}
}
@@ -151,7 +149,6 @@ checkout_main(int argc, char **argv)
if (argc == 0) {
warnx("no input file");
(usage)();
- exit (1);
}
if ((username = getlogin()) == NULL)
@@ -229,6 +226,8 @@ checkout_usage(void)
"usage: co [-TV] [-ddate] [-f[rev]] [-I[rev]] [-kmode] [-l[rev]]\n"
" [-M[rev]] [-p[rev]] [-q[rev]] [-r[rev]] [-sstate]\n"
" [-u[rev]] [-w[user]] [-xsuffixes] [-ztz] file ...\n");
+
+ exit(1);
}
/*
Index: merge.c
===
RCS file: /cvs/src/usr.bin/rcs/merge.c,v
retrieving revision 1.7
diff -u -p -r1.7 merge.c
--- merge.c 23 Jul 2010 21:46:05 - 1.7
+++ merge.c 8 May 2014 20:04:11 -
@@ -77,7 +77,6 @@ merge_main(int argc, char **argv)
exit(0);
default:
(usage)();
- exit(D_ERROR);
}
}
argc -= optind;
@@ -86,7 +85,6 @@ merge_main(int argc, char **argv)
if (argc != 3) {
warnx("%s arguments", (argc < 3) ? "not enough" : "too many");
(usage)();
- exit(D_ERROR);
}
for (; labels < 3; labels++)
@@ -118,4 +116,6 @@ merge_usage(void)
{
(void)fprintf(stderr,
"usage: merge [-EepqV] [-L label] file1 file2 file3\n");
+
+ exit(D_ERROR);
}
Index: rcsclean.c
===
RCS file: /cvs/src/usr.bin/rcs/rcsclean.c,v
retrieving revision 1.52
diff -u -p -r1.52 rcsclean.c
--- rcsclean.c 28 Jul 2010 09:07:11 - 1.52
+++ rcsclean.c 8 May 2014 20:05:52 -
@@ -60,7 +60,6 @@ rcsclean_main(int argc, char **argv)
if (RCS_KWEXP_INVAL(kflag)) {
warnx("invalid RCS keyword substitution mode");
(usage)();
- exit(1);
}
break;
case 'n':
@@ -90,7 +89,6 @@ rcsclean_main(int argc, char **argv)
break;
default:
(usage)();
- exit(1);
}
}
@@ -104,7 +102,6 @@ rcsclean_main(int argc, char **argv)
if ((dirp = opendir(".")) == NULL) {
warn("opendir");
(usage)();
- exit(1);
}
while ((dp = readdir(dirp)) != NULL) {
@@ -127,6 +124,8 @@ rcsclean_usage(void)
fprintf(stderr,
"usage: rcsclean [-TV] [-kmode] [-n[rev]] [-q[rev]] [-r[rev]]\n"
"[-u[rev]] [-xsuffixes] [-ztz] [file ...]\n");
+
+ exit(1);
}
static void
Index: rcsdiff.c
===
RCS file: /cvs/src/usr.bin/rcs/rcsdiff.c,v
retrieving revision 1.79
diff -u -p -r1.79 rcsdiff.c
--- rcsdiff.c 16 A
Re: Support for LC_TIME
Hi,
Marc Espie wrote on Thu, May 08, 2014 at 07:20:52PM +0200:
> On Thu, May 08, 2014 at 12:07:30PM +0200, Stefan Sperling wrote:
>> On Wed, May 07, 2014 at 07:44:51PM +0200, Ingo Schwarze wrote:
>>> While LC_CTYPE and LC_COLLATE make some sense, LC_MONETARY, LC_NUMERIC,
>>> and LC_TIME are badly overengineered, pointless bloat, causing nothing
>>> but surprising, erratic behaviour and portability problems when trying
>>> to parse output from programs. I think this should be rejected outright
>>> and you should stop wasting your time on it.
>> They make sense for systems that try to provide full i18n.
>> Of course, we don't try to provide i18n, at least not for the
>> base system which is English only. So they don't really make
>> sense *for OpenBSD*.
> ???
>
> Basic support for that stuff makes sense, as part of a *full* libc.
> Not surprisingly, Antoine is for providing LC_* support. So am I.
>
> This has little to do with "base OpenBSD", everything to do with "enough
> stuff to be able to compile reasonable portable software on OpenBSD
> without needing to patch left and right".
I don't see how any software might need patching if we continue
to ignore LC_TIME, just like we do now. It's just as if the user
never sets LC_TIME, which the standard specifically says *any*
software must cope with.
> As for portability issues: programs stay with the C locale *in any case*
> unless they do setlocale("") right at the start,
And that's what arch(1), at(1), awk(1), basename(1), calendar(1),
cat(1), chmod(1), cmp(1), cp(1), cron(8), csh(1), cut(1), date(1),
dig(1), dirname(1), env(1), expr(1), fmt(1), getconf(1), less(1),
logname(1), mandoc(1), mg(1), mkdir(1), mknod(8), nice(1), nl(1),
printf(1), rm(1), rmdir(1), sleep(1), sftp(1), sort(1), sudo(8),
tee(1), touch(1), tmux(1), uname(1), uudecode(1), vi(1), wc(1),
which(1), who(1), xargs(1) already do, right now.
Reliable and secure shell scripting will certainly be fun in
that LC_TIME ridden world.
> in which case they explicitly say "yes, I want to be localized".
> So, from that point of view the portability issues are minimal
> (yes, I'm aware of the can of worms that threads+locale may open).
>> That said, I don't have a general problem with adding other locale
>> categories. I believe LC_TIME would provide a useful testbed for
>> eventually switching all our locales to the localedef format
>> (including LC_CTYPE). Alas, the proposed diff does something else,
>> and unfortunately I don't have enough time for a detailed rabbit
>> hole discussion and review with a lot of back-and-forth that we
>> had when discussing similar diffs in the past.
> THAT on the other hand is the issue at hand... chronic time shortage
> to be certain that what we do for locales isn't dangerous...
I don't doubt it *will* cause trouble even if it were done in the
so-called "right" way, because it's the basic design that is broken,
not just some implementation. The concept is utterly wrong because
it does i18n *at the wrong level*, that is, not just in high level
graphical user interfaces, where it is merely annoying but doesn't
break much, but also at the system level, where it is nothing but
harmful. And that layering violation is a direct consequence of
having this code at the wrong level. No wonder it breaks everything
if it infects the C library including such functions as (according
to POSIX)
- LC_NUMERIC changing the radix character in strtod(3), printf(3), scanf(3)
- LC_TIME changing what strftime(3), strptime(3) and getdate(3) do,
up to including non-ASCII characters into library system messages
- LC_MESSAGES changing what strerror(3) does
- ...
Look here:
schwarze@donnerwolke:~$ uname
Linux
schwarze@donnerwolke:~$ locale
LANG=
LANGUAGE=
LC_CTYPE="de_DE.UTF-8"
LC_NUMERIC="de_DE.UTF-8"
LC_TIME="de_DE.UTF-8"
LC_COLLATE="de_DE.UTF-8"
LC_MONETARY="de_DE.UTF-8"
LC_MESSAGES="de_DE.UTF-8"
LC_PAPER="de_DE.UTF-8"
LC_NAME="de_DE.UTF-8"
LC_ADDRESS="de_DE.UTF-8"
LC_TELEPHONE="de_DE.UTF-8"
LC_MEASUREMENT="de_DE.UTF-8"
LC_IDENTIFICATION="de_DE.UTF-8"
LC_ALL=de_DE.UTF-8
schwarze@donnerwolke:~$ ls -l .xsession-errors
-rw--- 1 schwarze schwarze 89221 28. MÃ €r 00:04 .xsession-errors
(Blank inserted for clarity). Good luck parsing such abominations,
or as a system administrator, handling problem reports from users when
such stuff causes scripts to break.
Then again, given that this isn't going forward right now anyway,
maybe there is no need to waste time fighting back just yet.
Yours,
Ingo
Re: sparc64: problem after trap table takeover under QEMU
> Date: Thu, 08 May 2014 14:44:30 +0100 > From: Mark Cave-Ayland > > On 06/05/14 19:18, Mark Cave-Ayland wrote: Hi Mark, Interesting to see sparc64 support in QEMU. > > As soon as I step into address 0x1001804 then this is where things start > > to go wrong; the TLB (TTE) entry for 0x180 which is accessed by %sp > > is marked as privileged, but ASI 0x11 is user access only. QEMU's > > current behaviour for this is to generate a datafault for the page at > > 0x180 which seems to get all the way through to the retry at the end > > of winfixsave, but then hits the breakpoint trap above when executing > > the retry. > > I've finally located the source of this bug thanks to more testing, > which showed that OpenBSD 4.9 was surprisingly also able to boot > (something I missed this in my original bisection). This allowed me to > track down what was happening fairly easily. The problem is caused by > the fact that 0x180 has *two* mappings in the TLB and the way in > which QEMU resolves them. > > Compare the state of the TLB when the fill_0_normal trap occurs on > OpenBSD 5.5 (faults, incorrect) and OpenBSD 4.9 (no fault, correct): > > > OpenBSD 5.5: > > (qemu) info tlb > MMU contexts: Primary: 0, Secondary: 0 > DMMU dump > ... > [14] VA: 180, PA: f40, 4M, priv, RW, locked, ctx 0 local > ... > [42] VA: 180, PA: f40, 8k, user, RW, unlocked, ctx 0 local > ... > > OpenBSD 4.9: > > (qemu) info tlb > MMU contexts: Primary: 0, Secondary: 0 > DMMU dump > ... > [08] VA: 180, PA: f40, 8k, user, RW, unlocked, ctx 0 local > ... > [14] VA: 180, PA: f40, 4M, priv, RW, locked, ctx 0 local > ... > > > The bug occurs because the QEMU TLB algorithm currently searches the TLB > *in order* starting from entry 0 until it finds a VA match. > > In the OpenBSD 5.5 case, the first mapping it finds is the 4M privileged > mapping, and so the fill_0_normal trap which uses user ASI 0x11 faults > due to not being privileged. This is in contrast to the OpenBSD 4.9 case > where the first mapping it finds is the 8K unprivileged mapping, hence > the fill_0_normal trap succeeds and we proceed to boot. > > Does anyone know how real hardware resolves conflicts between multiple > TLB entries with the same VA? My guess would be that the smaller 8K > mapping should take priority, but the documentation in relation to > address aliasing is fairly non-existent so I wondering if there are any > other rules relating to whether privileged mappings should take priority > or not? Once the behaviour is known, it will be fairly easy to fix up > QEMU to match. I don;t know how the real hardware behaves. But it certainly is the intention that the 4M "locked" mapping gets used as soon as we've taken over the trap table. Not sure where the 8K mapping is coming from. > Finally it does raise an eyebrow that the first window trap taken when > the kernel takes over the trap table is a fill_0_normal *user* trap, > particularly when it's against an *unlocked* TLB entry which could > potentially could have been evicted beforehand. It might be worth > double-checking as to whether this is the intended behaviour or not. Right. It certainly isn't the intention that we end up a fill_0_normal at this point. Perhaps %wstate is initialized differently in QEMU than on real hardware? The OpenBSD bootstrap code does set %wstate appropriately immediately after taking over the trap table. We can't really do this earlier since we don't know the conventions used by the spill and fill handlers provided by the firmware. But it looks like a Sun Fire T2000 actually initializes %wstate to 0. So perhaps we're just getting lucky on real hardware that the prom code doesn't spill our trap frame and therefore we don't have to fill it again.
Re: Support for LC_TIME
On Thu, May 08, 2014 at 12:07:30PM +0200, Stefan Sperling wrote:
> On Wed, May 07, 2014 at 07:44:51PM +0200, Ingo Schwarze wrote:
> > While LC_CTYPE and LC_COLLATE make some sense, LC_MONETARY, LC_NUMERIC,
> > and LC_TIME are badly overengineered, pointless bloat, causing nothing
> > but surprising, erratic behaviour and portability problems when trying
> > to parse output from programs. I think this should be rejected outright
> > and you should stop wasting your time on it.
>
> They make sense for systems that try to provide full i18n.
> Of course, we don't try to provide i18n, at least not for the base system
> which is English only. So they don't really make sense *for OpenBSD*.
???
Basic support for that stuff makes sense, as part of a *full* libc.
Not surprisingly, Antoine is for providing LC_* support. So am I.
This has little to do with "base OpenBSD", everything to do with "enough
stuff to be able to compile reasonable portable software on OpenBSD
without needing to patch left and right".
As for portability issues: programs stay with the C locale *in any case*
unless they do setlocale("") right at the start, in which case they
explicitly say "yes, I want to be localized". So, from that point of view
the portability issues are minimal (yes, I'm aware of the can of worms
that threads+locale may open).
> That said, I don't have a general problem with adding other locale categories.
> I believe LC_TIME would provide a useful testbed for eventually switching all
> our
> locales to the localedef format (including LC_CTYPE). Alas, the proposed diff
> does something else, and unfortunately I don't have enough time for a detailed
> rabbit hole discussion and review with a lot of back-and-forth that we had
> when
> discussing similar diffs in the past.
THAT on the other hand is the issue at hand... chronic time shortage to be
certain that what we do for locales isn't dangerous...
Re: (int)sizeof in smtpd
On Thu, May 08, 2014 at 18:43, Alexandre Ratchov wrote: > On Thu, May 08, 2014 at 12:35:56PM -0400, Ted Unangst wrote: >> This is wrong in several ways. >> >> Never cast sizeof down, always cast the comparison variable up. >> >> I'll specifically call out this change: >> >> -if (snprintf(buf, sizeof(buf)) >= (int)sizeof(buf)) >> +if ((size_t)snprintf(buf, sizeof(buf)) >= sizeof(buf)) >> > > note that the >= operator promotes int to size_t, which makes the > cast useless and could permit lines to be shortened. Yes, I did that first, but somebody added -Wsign-compare to the Makefile. Silencing that warning is the root of a lot of evil. The cure is usually worse than the disease.
Re: (int)sizeof in smtpd
On 08 May 2014, at 18:43, Alexandre Ratchov wrote: > On Thu, May 08, 2014 at 12:35:56PM -0400, Ted Unangst wrote: >> This is wrong in several ways. >> >> Never cast sizeof down, always cast the comparison variable up. >> >> I'll specifically call out this change: >> >> -if (snprintf(buf, sizeof(buf)) >= (int)sizeof(buf)) >> +if ((size_t)snprintf(buf, sizeof(buf)) >= sizeof(buf)) >> > > note that the >= operator promotes int to size_t, which makes the > cast useless and could permit lines to be shortened. Wouldn't that produce a signed vs. unsigned comparison?
Re: (int)sizeof in smtpd
On Thu, May 08, 2014 at 12:35:56PM -0400, Ted Unangst wrote: > This is wrong in several ways. > > Never cast sizeof down, always cast the comparison variable up. > > I'll specifically call out this change: > > - if (snprintf(buf, sizeof(buf)) >= (int)sizeof(buf)) > + if ((size_t)snprintf(buf, sizeof(buf)) >= sizeof(buf)) > note that the >= operator promotes int to size_t, which makes the cast useless and could permit lines to be shortened.
(int)sizeof in smtpd
This is wrong in several ways.
Never cast sizeof down, always cast the comparison variable up.
I'll specifically call out this change:
- if (snprintf(buf, sizeof(buf)) >= (int)sizeof(buf))
+ if ((size_t)snprintf(buf, sizeof(buf)) >= sizeof(buf))
The top way fails when snprintf returns -1 (which doesn't happen, but
still) whereas the bottom way does handle it.
(yes, it's stupid that snprintf returns an int, but that's not a good
excuse. two stupids don't make a smart. :))
Index: expand.c
===
RCS file: /cvs/src/usr.sbin/smtpd/expand.c,v
retrieving revision 1.26
diff -u -p -r1.26 expand.c
--- expand.c19 Apr 2014 12:43:19 - 1.26
+++ expand.c8 May 2014 16:28:15 -
@@ -193,12 +193,14 @@ static int
expand_line_split(char **line, char **ret)
{
static char buffer[SMTPD_MAXLINESIZE];
- int esc, i, dq, sq;
+ int esc, dq, sq;
+ size_t i;
char *s;
memset(buffer, 0, sizeof buffer);
- esc = dq = sq = i = 0;
- for (s = *line; (*s) && (i < (int)sizeof(buffer)); ++s) {
+ esc = dq = sq = 0;
+ i = 0;
+ for (s = *line; (*s) && (i < sizeof(buffer)); ++s) {
if (esc) {
buffer[i++] = *s;
esc = 0;
Index: table.c
===
RCS file: /cvs/src/usr.sbin/smtpd/table.c,v
retrieving revision 1.15
diff -u -p -r1.15 table.c
--- table.c 19 Apr 2014 18:01:01 - 1.15
+++ table.c 8 May 2014 16:26:16 -
@@ -107,7 +107,7 @@ table_find(const char *name, const char
if (tag == NULL)
return dict_get(env->sc_tables_dict, name);
- if (snprintf(buf, sizeof(buf), "%s#%s", name, tag) >= (int)sizeof(buf))
{
+ if ((size_t)snprintf(buf, sizeof(buf), "%s#%s", name, tag) >=
sizeof(buf)) {
log_warnx("warn: table name too long: %s#%s", name, tag);
return (NULL);
}
@@ -194,8 +194,8 @@ table_create(const char *backend, const
struct stat sb;
if (name && tag) {
- if (snprintf(buf, sizeof(buf), "%s#%s", name, tag)
- >= (int)sizeof(buf))
+ if ((size_t)snprintf(buf, sizeof(buf), "%s#%s", name, tag) >=
+ sizeof(buf))
errx(1, "table_create: name too long \"%s#%s\"",
name, tag);
name = buf;
@@ -205,8 +205,8 @@ table_create(const char *backend, const
errx(1, "table_create: table \"%s\" already defined", name);
if ((tb = table_backend_lookup(backend)) == NULL) {
- if (snprintf(path, sizeof(path), PATH_TABLES "/table-%s",
- backend) >= (int)sizeof(path)) {
+ if ((size_t)snprintf(path, sizeof(path), PATH_TABLES
"/table-%s",
+ backend) >= sizeof(path)) {
errx(1, "table_create: path too long \""
PATH_TABLES "/table-%s\"", backend);
}
@@ -644,14 +644,14 @@ table_dump_lookup(enum table_service s,
case K_DOMAIN:
ret = snprintf(buf, sizeof(buf), "%s", lk->domain.name);
- if (ret == -1 || ret >= (int)sizeof (buf))
+ if (ret == -1 || (size_t)ret >= sizeof (buf))
goto err;
break;
case K_CREDENTIALS:
ret = snprintf(buf, sizeof(buf), "%s:%s",
lk->creds.username, lk->creds.password);
- if (ret == -1 || ret >= (int)sizeof (buf))
+ if (ret == -1 || (size_t)ret >= sizeof (buf))
goto err;
break;
@@ -659,7 +659,7 @@ table_dump_lookup(enum table_service s,
ret = snprintf(buf, sizeof(buf), "%s/%d",
sockaddr_to_text((struct sockaddr *)&lk->netaddr.ss),
lk->netaddr.bits);
- if (ret == -1 || ret >= (int)sizeof (buf))
+ if (ret == -1 || (size_t)ret >= sizeof (buf))
goto err;
break;
@@ -669,14 +669,14 @@ table_dump_lookup(enum table_service s,
lk->userinfo.uid,
lk->userinfo.gid,
lk->userinfo.directory);
- if (ret == -1 || ret >= (int)sizeof (buf))
+ if (ret == -1 || (size_t)ret >= sizeof (buf))
goto err;
break;
case K_SOURCE:
ret = snprintf(buf, sizeof(buf), "%s",
ss_to_text(&lk->source.addr));
- if (ret == -1 || ret >= (int)sizeof (buf))
+ if (ret == -1 || (size_t)ret >= sizeof (buf))
goto err;
break;
@@ -684,14 +684,14 @@ table_dump_lookup(enum table_service s,
Re: sparc64: problem after trap table takeover under QEMU
On 06/05/14 19:18, Mark Cave-Ayland wrote: (cut) As soon as I step into address 0x1001804 then this is where things start to go wrong; the TLB (TTE) entry for 0x180 which is accessed by %sp is marked as privileged, but ASI 0x11 is user access only. QEMU's current behaviour for this is to generate a datafault for the page at 0x180 which seems to get all the way through to the retry at the end of winfixsave, but then hits the breakpoint trap above when executing the retry. I've finally located the source of this bug thanks to more testing, which showed that OpenBSD 4.9 was surprisingly also able to boot (something I missed this in my original bisection). This allowed me to track down what was happening fairly easily. The problem is caused by the fact that 0x180 has *two* mappings in the TLB and the way in which QEMU resolves them. Compare the state of the TLB when the fill_0_normal trap occurs on OpenBSD 5.5 (faults, incorrect) and OpenBSD 4.9 (no fault, correct): OpenBSD 5.5: (qemu) info tlb MMU contexts: Primary: 0, Secondary: 0 DMMU dump ... [14] VA: 180, PA: f40, 4M, priv, RW, locked, ctx 0 local ... [42] VA: 180, PA: f40, 8k, user, RW, unlocked, ctx 0 local ... OpenBSD 4.9: (qemu) info tlb MMU contexts: Primary: 0, Secondary: 0 DMMU dump ... [08] VA: 180, PA: f40, 8k, user, RW, unlocked, ctx 0 local ... [14] VA: 180, PA: f40, 4M, priv, RW, locked, ctx 0 local ... The bug occurs because the QEMU TLB algorithm currently searches the TLB *in order* starting from entry 0 until it finds a VA match. In the OpenBSD 5.5 case, the first mapping it finds is the 4M privileged mapping, and so the fill_0_normal trap which uses user ASI 0x11 faults due to not being privileged. This is in contrast to the OpenBSD 4.9 case where the first mapping it finds is the 8K unprivileged mapping, hence the fill_0_normal trap succeeds and we proceed to boot. Does anyone know how real hardware resolves conflicts between multiple TLB entries with the same VA? My guess would be that the smaller 8K mapping should take priority, but the documentation in relation to address aliasing is fairly non-existent so I wondering if there are any other rules relating to whether privileged mappings should take priority or not? Once the behaviour is known, it will be fairly easy to fix up QEMU to match. Finally it does raise an eyebrow that the first window trap taken when the kernel takes over the trap table is a fill_0_normal *user* trap, particularly when it's against an *unlocked* TLB entry which could potentially could have been evicted beforehand. It might be worth double-checking as to whether this is the intended behaviour or not. Kind regards, Mark.
[PATCH] renumber CARP virtual host ID range
Hi all,
Following a discussion on the NANOG mailing regarding the overlap
between MAC addresses assigned to VRRP [1] and virtual host IDs as used
in the CARP protocol, it was suggested to use a range dedicated to CARP
[2]. Ytti assigned 74-66-30-FF-FE-00 .. 74-66-30-FF-FE-FF.
* Improve coexistence with other redundancy protocols
* Patch does not provide backwards compatibilty
Kind regards,
Job
[1]: http://www.iana.org/assignments/ethernet-numbers
[2]: http://mailman.nanog.org/pipermail/nanog/2014-May/066959.html
===
RCS file: /cvs/src/sys/netinet/ip_carp.c,v
retrieving revision 1.229
diff -u -r1.229 ip_carp.c
--- src/sys/netinet/ip_carp.c 30 Apr 2014 10:04:33 - 1.229
+++ src/sys/netinet/ip_carp.c 8 May 2014 13:18:50 -
@@ -1759,16 +1759,19 @@
carp_set_vhe_enaddr(struct carp_vhost_entry *vhe)
{
struct carp_softc *sc = vhe->parent_sc;
-
+
+/* CARP MAC range donated by Saku Ytti
+ http://ytti.fi/.well-known/iru/obsd-carp.txt
+*/
if (vhe->vhid != 0 && sc->sc_carpdev) {
if (vhe->vhe_leader && sc->sc_balancing == CARP_BAL_IP)
- vhe->vhe_enaddr[0] = 1;
+ vhe->vhe_enaddr[0] = 0x75;
else
- vhe->vhe_enaddr[0] = 0;
- vhe->vhe_enaddr[1] = 0;
- vhe->vhe_enaddr[2] = 0x5e;
- vhe->vhe_enaddr[3] = 0;
- vhe->vhe_enaddr[4] = 1;
+ vhe->vhe_enaddr[0] = 0x74;
+ vhe->vhe_enaddr[1] = 0x66;
+ vhe->vhe_enaddr[2] = 0x30;
+ vhe->vhe_enaddr[3] = 0xff;
+ vhe->vhe_enaddr[4] = 0xfe;
vhe->vhe_enaddr[5] = vhe->vhid;
vhe->vhe_sdl.sdl_family = AF_LINK;
Re: [PATCH] rcs merge
On Thu, May 08, 2014 at 09:45:22AM +0200, Janne Johansson wrote: > Can't say if this was the motivation here, but some people like to put > constants before variables for comparisons so as to easily catch the > difference between > if (a = 5) ... > and > if (5 = a) .. > when you really meant if (a == 5). > You are right, some people like to see constants before variables for comparisions. But if you look later in the code you see something like "if (argc != 3)" or "for (; labels < 3; labels++)". As you can see the variables are the first parameter for comparisions. So this diff makes it more consistent what format is used, too. fritjof > > > 2014-05-08 0:13 GMT+02:00 Fritjof Bornebusch : > > > Hi tech, > > > > I think "labels >= 3" is more readable than "3 <= labels". > > > > fritjof > > > > Index: merge.c > > === > > RCS file: /cvs/src/usr.bin/rcs/merge.c,v > > retrieving revision 1.7 > > diff -u -p -r1.7 merge.c > > --- merge.c 23 Jul 2010 21:46:05 - 1.7 > > +++ merge.c 7 May 2014 22:10:06 - > > @@ -62,7 +62,7 @@ merge_main(int argc, char **argv) > > flags |= MERGE_EFLAG; > > break; > > case 'L': > > - if (3 <= labels) > > + if (labels >= 3) > > errx(D_ERROR, "too many -L options"); > > label[labels++] = optarg; > > break; > > > > > > > -- > May the most significant bit of your life be positive.
Re: Support for LC_TIME
On Wed, May 07, 2014 at 07:44:51PM +0200, Ingo Schwarze wrote: > While LC_CTYPE and LC_COLLATE make some sense, LC_MONETARY, LC_NUMERIC, > and LC_TIME are badly overengineered, pointless bloat, causing nothing > but surprising, erratic behaviour and portability problems when trying > to parse output from programs. I think this should be rejected outright > and you should stop wasting your time on it. They make sense for systems that try to provide full i18n. Of course, we don't try to provide i18n, at least not for the base system which is English only. So they don't really make sense *for OpenBSD*. That said, I don't have a general problem with adding other locale categories. I believe LC_TIME would provide a useful testbed for eventually switching all our locales to the localedef format (including LC_CTYPE). Alas, the proposed diff does something else, and unfortunately I don't have enough time for a detailed rabbit hole discussion and review with a lot of back-and-forth that we had when discussing similar diffs in the past.
Re: [PATCH] rcs merge
Can't say if this was the motivation here, but some people like to put constants before variables for comparisons so as to easily catch the difference between if (a = 5) ... and if (5 = a) .. when you really meant if (a == 5). 2014-05-08 0:13 GMT+02:00 Fritjof Bornebusch : > Hi tech, > > I think "labels >= 3" is more readable than "3 <= labels". > > fritjof > > Index: merge.c > === > RCS file: /cvs/src/usr.bin/rcs/merge.c,v > retrieving revision 1.7 > diff -u -p -r1.7 merge.c > --- merge.c 23 Jul 2010 21:46:05 - 1.7 > +++ merge.c 7 May 2014 22:10:06 - > @@ -62,7 +62,7 @@ merge_main(int argc, char **argv) > flags |= MERGE_EFLAG; > break; > case 'L': > - if (3 <= labels) > + if (labels >= 3) > errx(D_ERROR, "too many -L options"); > label[labels++] = optarg; > break; > > -- May the most significant bit of your life be positive.
Re: [PATCH]: Convert select() to poll() in amd/info_hes.c
On Wed, May 07, 2014 at 12:13:42PM -0700, patrick keshishian wrote:
> On Wed, May 07, 2014 at 02:47:32PM +0100, Dimitris Papastamos wrote:
> > Hi,
> >
> > This piece of code now uses poll() instead of select().
> > I have not got round to test this yet, but I will as soon as I have
> > a working setup.
> >
> > Thoughts?
> >
> > ===
> > RCS file: /cvs/src/usr.sbin/amd/amd/info_hes.c,v
> > retrieving revision 1.13
> > diff -u -p -r1.13 info_hes.c
> > --- info_hes.c 24 Feb 2012 06:19:00 - 1.13
> > +++ info_hes.c 7 May 2014 13:38:21 -
> > @@ -66,7 +66,7 @@
> > #endif
> >
> > static int soacnt;
> > -static struct timeval hs_timeout;
> > +static int hs_timeout;
> > static int servernum;
> > #endif /* HAS_HESIOD_RELOAD */
> >
> > @@ -261,12 +261,11 @@ hs_res_send(char *buf, int buflen, char
> > */
> > for (retry = _res.retry; retry > 0; retry--) {
> > for (ns = 0; ns < hs_nscount; ns++) {
> > - hs_timeout.tv_sec =
> > - (_res.retrans << (_res.retry - retry))
> > - / hs_nscount;
> > - if (hs_timeout.tv_sec <= 0)
> > - hs_timeout.tv_sec = 1;
> > - hs_timeout.tv_usec = 0;
> > + hs_timeout = _res.retrans << (_res.retry - retry);
> > + hs_timeout /= hs_nscount;
> > + hs_timeout *= 1000;
> > + if (hs_timeout <= 0)
> > + hs_timeout = 1000;
>
> I imagine the values used here are small in scale, but
> I'm not sure. Any concerns of overflow given the new
> code is:
>
> - using an int type for calculations and storage vs
>time_t in the original.
> - breaking the bit-shift, divide and assign statement
>into two: a) bit-shift / assign, b) divide / assign.
What about using ppoll() instead?
