Re: Length of wmesg for condvar?
On Sun, 8 Aug 2010 17:23:23 -0700 (PDT) Paul Goyette p...@whooppee.com wrote: Should these be changed? Are there any adverse effects from having a wmesg longer than 8 characters? It seems to me that the exporter of those use strncpy() (i.e. kern/init_sysctl.c) and that the structures use WMESGLEN and KI_WMESGLEN both defined as 8. So other than inadvertently truncated names it at least should not cause corruption, but I think that truncated names could also be problematic when trying to distinguish two strings starting with the same 8 characters (is that likely now)? Especially when the only thing that differs between two states is some suffix like rd and rw... After all, those are intended for humans :) -- Matt
Re: Using coccinelle for (quick?) syntax fixing
On Sun, 8 Aug 2010 14:45:59 -0400, Matthew Mondor mm_li...@pulsar-zone.net wrote: On Sun, 08 Aug 2010 18:05:11 +0200 Jean-Yves Migeon jeanyves.mig...@free.fr wrote: Opinions? Any interest in it? My intent is to put NetBSD specific scripts on wiki.n.o, and provide links for more generic ones. That seems like a handy tool to save time and avoid a number of typos, if it's used right. Thanks for sharing, I personally didn't know Coccinelle. And example scripts can often be more useful than plain documentation, especially if it's in a WIP state (I liked that they showed in a few lines why it's better than sed :)) Yeah, the documentation is a WIP, and is rather difficult to follow; starting with SmPL grammar is one thing, but some options are clearly not documented, and hard to guess (I am not at all familiar with ocaml). I spent approx. 1 hour to figure out how I could print file name and line with the aprint thing, and never found a solution for pattern matching on expression (detect %s: ). Examples on the site _do_ help there. It is 'error-prone, in the sense that it can raise false positives. But when you get more familiar with it, you can either fix the cocci patch (easy for __arraycount, I missed one of the cases... less obvious for aprint stuff), and proof read the generated patch. I used these examples to get familiar with it; it starts getting useful when you try to find out buggy code, like double free() in the same function, mutex_exit() missing in a branch before returning, etc. -- Jean-Yves Migeon jeanyves.mig...@free.fr
Re: Using coccinelle for (quick?) syntax fixing
On Mon Aug 09 2010 at 11:20:29 +0200, Jean-Yves Migeon wrote: It is 'error-prone, in the sense that it can raise false positives. But when you get more familiar with it, you can either fix the cocci patch (easy for __arraycount, I missed one of the cases... less obvious for aprint stuff), and proof read the generated patch. I really dislike untested wide-angle churn, especially if there is 0 measurable gain. Converting code to __arraycount is a prime example. The only benefit of __arraycount is avoiding typing and therefore typos. Neither of those apply when doing a churn. (there are subjective beauty values, but every C programmer knows the sizeof/sizeof idiom, which is more than what can be said about __arraycount) Examples of measurable benefit are good. Encouraging churn is less good, even if spatch-churn is a million times better than sed-churn. I used these examples to get familiar with it; it starts getting useful when you try to find out buggy code, like double free() in the same function, mutex_exit() missing in a branch before returning, etc. Static analysis is good. However, it might take quite a bit of effort to get the rules general enough so that they trigger in more than one file and specific enough so that you don't get too many false positives. Just to give an example, the ffs allocator routines don't release the lock in an error branch. I remember coccinelle had problems with cpp. Any code which uses macros to skip C syntax will fail silently. procfs comes to mind here. Also, I remember it using so much memory when given our kernel source that I could not finish a rototill and had to use it in combo with find/grep. That said, if $someone can produce a set of rules which showably find bugs in NetBSD code and do not produce a lot of false positives, I'm very interested in seeing nightly runs. ... especially if there are no TAILQ false positives ;)
Re: Using coccinelle for (quick?) syntax fixing
On Mon, 9 Aug 2010 13:27:38 +0300, Antti Kantee po...@cs.hut.fi wrote: I really dislike untested wide-angle churn, especially if there is 0 measurable gain. Converting code to __arraycount is a prime example. The only benefit of __arraycount is avoiding typing and therefore typos. Neither of those apply when doing a churn. (there are subjective beauty values, but every C programmer knows the sizeof/sizeof idiom, which is more than what can be said about __arraycount) arraycount is just one example; you could say the same about roundup/howmany/min/max/... For arraycount, the use is limited; I agree. However, I would prefer to have code using roundup/MIN/MAX rather than rewriting them down. It tends to be harder to read, no matter the developer's expertise can be. Examples of measurable benefit are good. Encouraging churn is less good, even if spatch-churn is a million times better than sed-churn. Well, that's the eternal if it ain't broke, don't fix it debate. I don't know about what you mean by measurable benefit. Does code quality, DRY, or KNF fit in? I hardly see how the thing can go wrong in this /very/ simple case though: dependency on cdefs, exact semantic match. Anyway, that's not the purpose here. I used these examples to get familiar with it; it starts getting useful when you try to find out buggy code, like double free() in the same function, mutex_exit() missing in a branch before returning, etc. Static analysis is good. However, it might take quite a bit of effort to get the rules general enough so that they trigger in more than one file and specific enough so that you don't get too many false positives. Just to give an example, the ffs allocator routines don't release the lock in an error branch. That's not because there are specific cases where the tool has limited use that it does not fit the rest :) I remember coccinelle had problems with cpp. Any code which uses macros to skip C syntax will fail silently. procfs comes to mind here. Also, I remember it using so much memory when given our kernel source that I could not finish a rototill and had to use it in combo with find/grep. Indeed, using spatch -dir /usr/cvs/src is quite violent. And does not scale very well when the cases are too general, and need heavy pattern lookup. That said, if $someone can produce a set of rules which showably find bugs in NetBSD code and do not produce a lot of false positives, I'm very interested in seeing nightly runs. Alright, let's get a bit more practical. Warning: patch may not apply cleanly, and I am working with a month-old src: - logical not with bitwise (attached: notvsand.diff) This one should be checked, I am not familiar with this code. - more serious: NULL check then dereference (attached: nullderef.diff) IMHO, the last ones would be easier to spot with if (foo == NULL)... instead of having if (!foo)... Clarity does help (guess it did not for the other half, anyway :/ ) ... especially if there are no TAILQ false positives ;) Only true negatives with that one :p Purpose was to gather opinions (if it got wildly rejected, I would have stop doing anything with it now). Note that I view coccinelle more like a tool making rototilling easier than full blown static analyzer; there are probably better ones out there (clang). -- Jean-Yves Migeon jeanyves.mig...@free.fr notvsand.diff Description: Binary data nullderef.diff Description: Binary data
Re: Length of wmesg for condvar?
On Mon, Aug 09, 2010 at 02:38:32AM -0400, Matthew Mondor wrote: On Sun, 8 Aug 2010 17:23:23 -0700 (PDT) Paul Goyette p...@whooppee.com wrote: Should these be changed? Are there any adverse effects from having a wmesg longer than 8 characters? It seems to me that the exporter of those use strncpy() strncpy() is unlikely to do what is expected of it. strlcpy() is probably rather better. David -- David Laight: da...@l8s.co.uk
Re: Using coccinelle for (quick?) syntax fixing
On 09.08.2010 18:45, Antti Kantee wrote: On Mon Aug 09 2010 at 18:19:28 +0200, Jean-Yves Migeon wrote: That said, if $someone can produce a set of rules which showably find bugs in NetBSD code and do not produce a lot of false positives, I'm very interested in seeing nightly runs. Alright, let's get a bit more practical. Warning: patch may not apply cleanly, and I am working with a month-old src: - logical not with bitwise (attached: notvsand.diff) This one should be checked, I am not familiar with this code. Those definitely look fishy. I'm surprised gcc doesn't give a warning. Does not seem to... - more serious: NULL check then dereference (attached: nullderef.diff) IMHO, the last ones would be easier to spot with if (foo == NULL)... instead of having if (!foo)... Clarity does help (guess it did not for the other half, anyway :/ ) Well, at least the first one is under #ifdef __FreeBSD__, and would panic anyway already in malloc ;) Keep looking down a bit :) IMHO, all the rest are real bugs, and concern kernel directly. It affects parts I am sadly not experienced enough with, so I wouldn't commit without prior approval. Note also that I applied the spatch against sys/; the rest of src could get a scan. But I would prefer to look for other static analyzers first, perhaps there are more suitable (and faster) ones. IIRC, there were some private discussions earlier (year or so ago?) and it was suggested that rototills should accompany a public spatch. But given that in practise applying the spatch is usually a combination of using spatch, sed, etc., there would need to be accompanying documentation too. Anyway, someone needs to work out the details and drive the issue. I think your examples are useful to run every now and then in a static analyzer capacity. Just because there are better ones, doesn't mean others are obsolete, especially if the results are different. This requires maintenance, and having multiple tools each raising false positives can only get semi-automated. Besides, avoiding false positive may get the opposite result, masking true ones. -- Jean-Yves Migeon jeanyves.mig...@free.fr
Re: Length of wmesg for condvar?
On Mon, 9 Aug 2010, Matthew Mondor wrote: On Sun, 8 Aug 2010 17:23:23 -0700 (PDT) Paul Goyette p...@whooppee.com wrote: Should these be changed? Are there any adverse effects from having a wmesg longer than 8 characters? It seems to me that the exporter of those use strncpy() (i.e. kern/init_sysctl.c) and that the structures use WMESGLEN and KI_WMESGLEN both defined as 8. So other than inadvertently truncated names it at least should not cause corruption, but I think that truncated names could also be problematic when trying to distinguish two strings starting with the same 8 characters (is that likely now)? Especially when the only thing that differs between two states is some suffix like rd and rw... After all, those are intended for humans :) Does anyone object to my going through and coming up with shorter names (= 8 chars) for these condvars? I'll leave changing the exporters from strncpy() to strlcpy() for another day. - | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | Customer Service | FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette at juniper.net | | Kernel Developer | | pgoyette at netbsd.org | -
re: Length of wmesg for condvar?
On Mon, Aug 09, 2010 at 02:38:32AM -0400, Matthew Mondor wrote: On Sun, 8 Aug 2010 17:23:23 -0700 (PDT) Paul Goyette p...@whooppee.com wrote: Should these be changed? Are there any adverse effects from having a wmesg longer than 8 characters? It seems to me that the exporter of those use strncpy() strncpy() is unlikely to do what is expected of it. strlcpy() is probably rather better. these are not (stored) as normal C-strings. if they're the full 8 chars, they'll have no nul. .mrg.
Re: Length of wmesg for condvar?
On Mon, 9 Aug 2010 22:21:02 +0100 David Laight da...@l8s.co.uk wrote: On Mon, Aug 09, 2010 at 02:02:51PM -0700, Paul Goyette wrote: Does anyone object to my going through and coming up with shorter names (= 8 chars) for these condvars? It is worth chcking whether they are displayed with a %.8s format (or similar) so that they don't need to be 0 terminated. Otherwise the names must be strictly less than 8 bytes. David -- David Laight: da...@l8s.co.uk That is worthy of concern, so I checked top and ps: top uses char wmesg[KI_WMESGLEN + 1]; strlcpy(wmesg, pp-p_wmesg, sizeof(wmesg)); ps uses strprintorsetwidth(v, l-l_wmesg, mode); v-width = min(v-width, KI_WMESGLEN); Thanks, -- Matt
Re: pchb@acpi
On Mon, Aug 02, 2010 at 06:42:11PM +0900, KIYOHARA Takashi wrote: Hi! Quentin, From: Quentin Garnier c...@cubidou.net Date: Sun, 1 Aug 2010 15:21:18 + On Sun, Aug 01, 2010 at 11:17:54PM +0900, KIYOHARA Takashi wrote: All recent PC has information on PCI in ACPI. We can attach pchb in acpi like a lot of other acpi devices. p...@acpi has been fairly supported in FreeBSD since before. ftp://ftp.netbsd.org/pub/NetBSD/misc/kiyohara/ia64/p...@acpi-support.diff Do you have anything further in mind? This patch is only dmesg cosmetics. No, I don't. My ia64 machine not need it. However it pass segment information to pci(4) in the future possibly. What kind of segment information? Attached is a preview of information that I supply to instances of pci(4), ppb(4), and cbb(4) through their device properties. The information will help them manage PCI address spaces and to program their address windows correctly, so that I can retire rbus and PCI_ADDR_FIXUP, whose heuristics are incomplete. Currently, I derive the information by scanning PCI Configuration Space. The system BIOS---be it OpenFirmware, ACPI, or something else---may supply similar information. Dave -- David Young OJC Technologies dyo...@ojctech.com Urbana, IL * (217) 278-3933 Properties for device `pci0': ?xml version=1.0 encoding=UTF-8? !DOCTYPE plist PUBLIC -//Apple Computer//DTD PLIST 1.0//EN http://www.apple.com/DTDs/PropertyList-1.0.dtd; plist version=1.0 dict keydevice-driver/key stringpci/string keydevice-unit/key integer0x0/integer keypci-resources/key dict keymemory/key dict keybios-reservations/key array dict keyaddress/key integer0xf200/integer keybus/key integer0x0/integer keydevice/key integer0x2/integer keyfunction/key integer0x0/integer keysize/key integer0x600/integer keytype/key stringwindow/string /dict dict keyaddress/key integer0xfc30/integer keybus/key integer0x0/integer keydevice/key integer0x3/integer keyfunction/key integer0x0/integer keysize/key integer0x30/integer keytype/key stringwindow/string /dict dict keyaddress/key integer0xf800/integer keybus/key integer0x0/integer keydevice/key integer0x1c/integer keyfunction/key integer0x0/integer keysize/key integer0x400/integer keytype/key stringwindow/string /dict dict keyaddress/key integer0xfc60/integer keybus/key integer0x0/integer keydevice/key integer0x1d/integer keyfunction/key integer0x7/integer keysize/key integer0x400/integer keytype/key stringBAR/string /dict dict keyaddress/key integer0xfc10/integer
Re: pchb@acpi
On Mon, 9 Aug 2010, David Young wrote: What kind of segment information? Attached is a preview of information that I supply to instances of pci(4), ppb(4), and cbb(4) through their device properties. The information will help them manage PCI address spaces and to program their address windows correctly, so that I can retire rbus and PCI_ADDR_FIXUP, whose heuristics are incomplete. Currently, I derive the information by scanning PCI Configuration Space. The system BIOS---be it OpenFirmware, ACPI, or something else---may supply similar information. Take a look at this: http://playground.sun.com/1275/bindings/pci/pci2_1.pdf I think it would make everyone's life easier if you could encode the properties as close to the 1275 bindings as possible. That would the amount of massaging needed to the properties OpenFirmware already provides. Eduardo
Re: Using coccinelle for (quick?) syntax fixing
Date:Mon, 09 Aug 2010 18:19:28 +0200 From:Jean-Yves Migeon jeanyves.mig...@free.fr Message-ID: e2568087e60b3c2943d03413cd7e5...@localhost I agree with pooka, no code changes to existing code just to make the code look better. | However, I would prefer to have code using roundup/MIN/MAX rather than | rewriting them down. It tends to be harder to read, no matter the | developer's expertise can be. If you're developing new code, or even making changes to existing code for some other reason, then fine - but changing the code just because you can't think of anything else to do, is not useful, however much you would prefer it if the code had been written in a different style. | Does code quality, DRY, or KNF fit in? No. If the code is there, and works as it is supposed to, then leave it alone. If it needs changing for some reason (adding features, removing old stuff that is no longer wanted, fixing bugs, improving performance, ...) then change it as needed. Otherwise, leave it alone. | I hardly see how the thing can go wrong in this /very/ | simple case though: dependency on cdefs, exact semantic match. One obvious example is that it makes it much harder in the future to pullups of fixes to code in this area to earlier NetBSD versions - the diffs show all of this meaningless churn as well as the real change. The same for anyone looking to see why something that used to work no longer does - you might be sure that your changes cannot possibly do any harm, but all someone else sees is a whole bunch of files changed between the version that worked last week, and the new version that doesn't work this week, that they're going to have to filter through to figure out what change actually broke things, even if your change was not responsible (and it probably wouldn't be) you've caused lots of needless unnecessary work. Please don't. kre ps: this has nothing whatever to do with the tool you're using to assist, whether this is done by hand, or automated, makes no real difference.
