Re: CVS commit: src/sys
On Sun, Nov 16, 2014 at 12:21 AM, Takeshi Nakayama t...@catvmics.ne.jp wrote:
Takeshi Nakayama t...@catvmics.ne.jp wrote
Justin Cormack jus...@specialbusservice.com wrote
Er, you can't do that.
1. It breaks the rump builds on most platforms
http://build.myriabit.eu:8012/waterfall as the prototypes dont match
eg see
http://build.myriabit.eu:8012/builders/ppc64-cross/builds/5585/steps/shell_3/logs/stdio
It seems that posix says 2nd arg of iconv(3) is char **, but
NetBSD's one is const char **.
2. There is no requirement that rump runs on a platform that has iconv
anyway, it may be running on bare metal, or non Posix platform.
Not sure what the intention was though - I am sure we can find a way
around it...
I would like to include this at least on NetBSD host since we don't
have kernel iconv and then mount_smbfs(8) is useless for filename
conversions.
So is it ok to add a compile-time option as below and define it
somewhere? Or are there any more appropriate make variables to
detect host OS?
On second thought, it seems user component can use __NetBSD__
definition, how about this change?
You also need to wrap the #include iconv.h in sys/netsmb/iconv.c in
#ifdef as well, as some platforms do not have the header.
How does it work when not using rump? Do we really need in kernel iconv?
Justin
-- Takeshi Nakayama
Index: netsmb_user.c
===
RCS file: /cvsroot/src/sys/rump/dev/lib/libnetsmb/netsmb_user.c,v
retrieving revision 1.1
diff -u -d -r1.1 netsmb_user.c
--- netsmb_user.c 15 Nov 2014 18:49:04 - 1.1
+++ netsmb_user.c 16 Nov 2014 00:21:28 -
@@ -36,6 +36,7 @@
int
rumpcomp_netsmb_iconv_open(const char *to, const char *from, void **handle)
{
+#ifdef __NetBSD__
iconv_t cd;
int rv;
@@ -49,11 +50,16 @@
}
return rumpuser_component_errtrans(rv);
+#else
+ /* fallback to use stub functions */
+ return 0;
+#endif
}
int
rumpcomp_netsmb_iconv_close(void *handle)
{
+#ifdef __NetBSD__
int rv;
if (iconv_close((iconv_t)handle) == -1)
@@ -62,12 +68,17 @@
rv = 0;
return rumpuser_component_errtrans(rv);
+#else
+ /* do nothing */
+ return 0;
+#endif
}
int
rumpcomp_netsmb_iconv_conv(void *handle, const char **inbuf,
size_t *inbytesleft, char **outbuf, size_t *outbytesleft)
{
+#ifdef __NetBSD__
int rv;
if (iconv((iconv_t)handle, inbuf, inbytesleft, outbuf, outbytesleft)
@@ -77,5 +88,9 @@
rv = 0;
return rumpuser_component_errtrans(rv);
+#else
+ /* do nothing */
+ return 0;
+#endif
}
#endif
Re: CVS commit: src/sys
On 15/11/14 23:46, Takeshi Nakayama wrote: Justin Cormack jus...@specialbusservice.com wrote Er, you can't do that. 1. It breaks the rump builds on most platforms http://build.myriabit.eu:8012/waterfall as the prototypes dont match eg see http://build.myriabit.eu:8012/builders/ppc64-cross/builds/5585/steps/shell_3/logs/stdio It seems that posix says 2nd arg of iconv(3) is char **, but NetBSD's one is const char **. 2. There is no requirement that rump runs on a platform that has iconv anyway, it may be running on bare metal, or non Posix platform. Not sure what the intention was though - I am sure we can find a way around it... I would like to include this at least on NetBSD host since we don't have kernel iconv and then mount_smbfs(8) is useless for filename conversions. So is it ok to add a compile-time option as below and define it somewhere? Or are there any more appropriate make variables to detect host OS? Is the effect that the smbfs driver has more functionality when it is run in a rump kernel than when it is run in the NetBSD kernel? That is not a good change. In addition to reason 2 cited above, there is a usability problem: mount with and without -o rump is supposed to provide the same functionality. If -o rump starts meaning with working iconv, it will make configuration and documentation difficult. While it is possible to add compile-time conditionals to the effect that your change does not per se break anything, I would rather see a fix which also works with mount_smbfs.
Re: CVS commit: src/sys/arch/atari/stand/installboot
Date: Sun, 16 Nov 2014 12:51:34 +0900 From: Izumi Tsutsui tsut...@ceres.dti.ne.jp My understanding is the strict aliasing rule is referred by compiler for reorder optimization etc. If the access via union is still invalid, why does the strict gcc48 no longer complain against it? It's access via two different pointers, which happen to have the same bits by virtue of being stored in a union -- the union doesn't actually do anything about aliasing of the pointed-to object, except probably confuse gcc. The point is that you can't get at one object in memory through two pointers of different types (except to get at a non-char object via a char pointer). The compiler sometimes tries to detect when you're violating this rule, but it can't always -- casting through void *, or passing the pointer through a union of pointer types, may suppress warnings, but will still violate the rule. It's TierII MD code, so maintainability is much more important than perfection (unless you will take maintainership of this installboot). I read the thread, but I'm not clear on how using memcpy in order to avoid undefined behaviour reduces maintainability. I don't see any byte ordering issues here that weren't present before. Currently it may work. But once we will try to make the MD installboot merged into MI usr.sbin/installboot, accessing variables via different types always confuse programmers who don't know actuall intention (which is the host endian value, which is the target endian value etc). Changing `*(uint16_t *)p = v' to `memcpy(p, v, 2)' doesn't change any of that. Seems to me if one wants to clarify the intention, one should add host16enc and target16enc and replace memcpy by whichever of those is appropriate.
Re: CVS commit: src/sys/arch/atari/stand/installboot
It's access via two different pointers, which happen to have the same bits by virtue of being stored in a union -- the union doesn't actually do anything about aliasing of the pointed-to object, except probably confuse gcc. The point is that you can't get at one object in memory through two pointers of different types (except to get at a non-char object via a char pointer). The compiler sometimes tries to detect when you're violating this rule, but it can't always -- casting through void *, or passing the pointer through a union of pointer types, may suppress warnings, but will still violate the rule. Then what's your point? Should we always strictly use memcpy even in MD code you won't maintain? If you really don't like current code, I'd still like to keep the original casts with -fno-strict-aliasing, instead of memcpy because my goal is just to switch m68k ports to using gcc48 by default in netbsd-7, not playing with modern compiler and C specifications. Changing `*(uint16_t *)p = v' to `memcpy(p, v, 2)' doesn't change any of that. The formar can be easily changed *(uint16_t *)p = htobe16(v); but the latter can't. You might be able to use functions like host16enc and target16enc for streaming data, but the target cksum is not stream but just calculated in uint16_t. --- Izumi Tsutsui
Re: CVS commit: src/sys/arch/atari/stand/installboot
On Nov 17, 1:37am, tsut...@ceres.dti.ne.jp (Izumi Tsutsui) wrote: -- Subject: Re: CVS commit: src/sys/arch/atari/stand/installboot | Then what's your point? | Should we always strictly use memcpy even in MD code you won't maintain? | | If you really don't like current code, I'd still like to keep | the original casts with -fno-strict-aliasing, instead of memcpy | because my goal is just to switch m68k ports to using gcc48 by default | in netbsd-7, not playing with modern compiler and C specifications. I don't understand why you are so much against the memcpy. This is what we were already doing with the cast (before it became undefined behavior) and the language definition mandates it. It is not like the compiler has to warn about pointer aliasing, or that -fno-strict-aliasing will work with every compiler... Or even in the next version of gcc. We fix the warnings to future-proof the code. | The formar can be easily changed | *(uint16_t *)p = htobe16(v); | but the latter can't. You might be able to use functions like | host16enc and target16enc for streaming data, but the target | cksum is not stream but just calculated in uint16_t. I am sure we can fix the code in a way that it is both readable and correct. Leaving it the way it was, is not an option. christos
Re: CVS commit: src/sys/arch/atari/stand/installboot
Date: Mon, 17 Nov 2014 01:37:37 +0900 From: Izumi Tsutsui tsut...@ceres.dti.ne.jp Changing `*(uint16_t *)p = v' to `memcpy(p, v, 2)' doesn't change any of that. The formar can be easily changed *(uint16_t *)p = htobe16(v); but the latter can't. You might be able to use functions like host16enc and target16enc for streaming data, but the target cksum is not stream but just calculated in uint16_t. So write this? host16enc(bb-bb_xxboot + 510, 0); host16enc(bb-bb_xxboot + 510, 0x1234 - abcksum(bb-bb_xxboot)); I don't see the problem.
Re: CVS commit: src/sys/arch/atari/stand/installboot
On Nov 16, 6:07pm, campbell+netbsd-source-change...@mumble.net (Taylor R Campbell) wrote: -- Subject: Re: CVS commit: src/sys/arch/atari/stand/installboot |Date: Mon, 17 Nov 2014 01:37:37 +0900 |From: Izumi Tsutsui tsut...@ceres.dti.ne.jp | | Changing `*(uint16_t *)p = v' to `memcpy(p, v, 2)' doesn't change any | of that. | |The formar can be easily changed | *(uint16_t *)p = htobe16(v); |but the latter can't. You might be able to use functions like |host16enc and target16enc for streaming data, but the target |cksum is not stream but just calculated in uint16_t. | | So write this? | | host16enc(bb-bb_xxboot + 510, 0); | host16enc(bb-bb_xxboot + 510, 0x1234 - abcksum(bb-bb_xxboot)); | | I don't see the problem. Yes be16enc() should work just fine, I think. christos
Re: CVS commit: src/sys/arch/atari/stand/installboot
christos@ wrote:
Yes be16enc() should work just fine, I think.
Then why don't you guys also complain to fix existing abcksum() function
which is called at the suggested memcpy?
---
/* set AHDI checksum */
sum = 0;
memcpy(bb-bb_xxboot + 255 * sizeof(sum), sum, sizeof(sum));
sum = 0x1234 - abcksum(bb-bb_xxboot);
memcpy(bb-bb_xxboot + 255 * sizeof(sum), sum, sizeof(sum));
:
static u_int
abcksum (void *bs)
{
u_int16_t sum = 0,
*st = (u_int16_t *)bs,
*end = (u_int16_t *)bs + 256;
while (st end)
sum += *st++;
return(sum);
}
---
I think the correct fix is to redefine a boot block structures as
a union of existing struct bootblock and uint16_t array in sys/bootblock.h,
but it would be done when we will merge it into MI installboot.
Fixing only a visible part you are shown in a patch to appease compiler
makes no sense even for correctness.
---
Izumi Tsutsui
re: CVS commit: src/distrib/atari/floppies/common
Izumi Tsutsui writes: Module Name: src Committed By: tsutsui Date: Sun Nov 16 11:54:29 UTC 2014 Modified Files: src/distrib/atari/floppies/common: Makefile.images Log Message: Use -Os -m68020-60 for DBG. It seems to generate smaller objects than -Os. gcc48 with -Os: -rwxr-xr-x 1 tsutsui wheel 1319596 Nov 16 20:50 obj.atari/instbin gcc48 with -Os -m68020-60 -rwxr-xr-x 1 tsutsui wheel 1314516 Nov 16 20:49 obj.atari/instbin This allows ever growing sysinst.fs still fit into 1440KB even with gcc48. Acually we need a real solution (ustarfs based floppies etc.) soon but we can work around at least for NetBSD 7.0. Should be pulled up to netbsd-7 (if NetBSD/m68k 7.0 will switch to gcc48). it might be nice to pull up anyway, so people can choose to use netbsd-7 in their own builds? :-) .mrg.
Re: CVS commit: src/sys/arch/atari/stand/installboot
On Nov 17, 5:50am, tsut...@ceres.dti.ne.jp (Izumi Tsutsui) wrote: -- Subject: Re: CVS commit: src/sys/arch/atari/stand/installboot | christos@ wrote: | | Yes be16enc() should work just fine, I think. | | Then why don't you guys also complain to fix existing abcksum() function | which is called at the suggested memcpy? Because it is legally converting a void * pointer. christos
re: CVS commit: src/sys
Ryota Ozaki writes: Module Name: src Committed By: ozaki-r Date: Sun Nov 16 16:20:01 UTC 2014 Modified Files: src/sys/arch/x86/pci: fwhrng.c src/sys/arch/x86/x86: via_padlock.c src/sys/dev/bluetooth: bcsp.c btkbd.c src/sys/dev/ic: nslm7x.c src/sys/dev/ir: irframe_tty.c src/sys/dev/isa: aps.c src/sys/dev/pci: pccbb.c src/sys/dev/pcmcia: btbc.c src/sys/dev/sdmmc: sdmmc.c src/sys/dev/wscons: wskbd.c src/sys/net: if_ecosubr.c Log Message: Replace callout_stop with callout_halt In order to call callout_destroy for a callout safely, we have to ensure the function of the callout is not running and pending. To do so, we should use callout_halt, not callout_stop. Discussed with martin@ and riastradh@. thanks. it might be nice if callout(9) was expanded to specifically warn against using callout_stop() before callout_destroy(). i suspect many of these should be pulled up to -5 and -6. .mrg.
re: CVS commit: src/sys/dev/videomode
Jared D. McNeill writes: Module Name: src Committed By: jmcneill Date: Mon Nov 17 00:46:04 UTC 2014 Modified Files: src/sys/dev/videomode: edid.c edidreg.h edidvar.h Log Message: Parse the extension block count field, and make it available in struct edid_info hmm, does this change the module ABI? ie, kernel version bump time? .mrg.
Re: CVS commit: src/sys
On Mon, Nov 17, 2014 at 6:58 AM, matthew green m...@eterna.com.au wrote: Ryota Ozaki writes: Module Name: src Committed By: ozaki-r Date: Sun Nov 16 16:20:01 UTC 2014 Modified Files: src/sys/arch/x86/pci: fwhrng.c src/sys/arch/x86/x86: via_padlock.c src/sys/dev/bluetooth: bcsp.c btkbd.c src/sys/dev/ic: nslm7x.c src/sys/dev/ir: irframe_tty.c src/sys/dev/isa: aps.c src/sys/dev/pci: pccbb.c src/sys/dev/pcmcia: btbc.c src/sys/dev/sdmmc: sdmmc.c src/sys/dev/wscons: wskbd.c src/sys/net: if_ecosubr.c Log Message: Replace callout_stop with callout_halt In order to call callout_destroy for a callout safely, we have to ensure the function of the callout is not running and pending. To do so, we should use callout_halt, not callout_stop. Discussed with martin@ and riastradh@. thanks. it might be nice if callout(9) was expanded to specifically warn against using callout_stop() before callout_destroy(). Okay, will do. i suspect many of these should be pulled up to -5 and -6. I can work on pullup-6 and pullup-7 but I'd appreciate it if someone does pullup-5 because I have no netbsd-5 tree in my local machine. Anyway another patch is under review so that please wait for the patch committed :) ozaki-r .mrg.
Re: CVS commit: src/sys/sys
On Sun, Nov 16, 2014 at 09:27:26PM -0500, Christos Zoulas wrote: Module Name: src Committed By: christos Date: Mon Nov 17 02:27:26 UTC 2014 Added Files: src/sys/sys: clock.h Log Message: PR/49207: Kamil Rytarowski: Centralize and rename a bunch of clock constants and inline functions. Why inline functions? None of these are used often or time critical. Besides: this broke the tools build, as you did not deal with tools/compat/subr_clock.h. Martin
