Community
Participate
Working Groups
Created attachment 198362 [details] Patch fixing various minor problems with StateStrings.java Identification: * LTTng - Linux Tracing Toolkit (Incubation) * According to org.eclipse.linuxtools.lttng/pom.xml, version 0.3.1 (ChangeLog stops at 2010-12-15) * Obtained from git.eclipse.org (org.eclipse.linuxtools.git, dated 2011-05-10) * org.eclipse.linuxtools.lttng/src/org/eclipse/linuxtools/lttng/state/StateStrings.java The attached patch fixes the following problems: 1) Not all enums declare their inName strings as "private final". 2) the Events.associate code for LTT_EVENT_FUNCTION_EXIT is duplicated from lines 334-339 to lines 341-346. Luckily this is harmless except for wasted CPU cycles. 3) there is no Events.associate code for LTT_EVENT_KPROBE, which may be unintended. I presume the parent should be LTT_CHANNEL_KPROBE_STATE, and that there are no children. 4) the Events.associate code for LTT_EVENT_KPROBE_TABLE is miscommented as LTT_EVENT_SYS_CALL_TABLE. 5) the code blocks of Events.associate are in somewhat different order than the Events enum itself, a mostly cosmetic annoyance. 6) LTT_EVENT_PAGE_FAULT_NOSEM_EXIT has no Parent set. I presume it should be LTT_CHANNEL_KERNEL. 7) LTT_EVENT_REQUEST_ISSUE adds the field LTT_FIELD_OPERATION twice (harmlessly). 8) Since every single member of the Events enum has its stateTransition flag set to True by Events.associate, it is simpler to set the default to True instead of False. The LTT_EVENT_SYSCALL_ENTRY comment seems to indicate this event also has an LTT_FIELD_IP field, but the Events.associate code only declares the LTT_FIELD_SYSCALL_ID field. Is the comment incorrect? LTT_EVENT_IRQ_ENTRY likewise declares in its comment that it "also receives fields kernel_mode, ip and handler" (i.e. LTT_FIELD_MODE(?), LTT_FIELD_IP and...which field?), but the code only adds the LTT_FIELD_IRQ_ID field. This also needs to be checked and clarified. org.eclipse.linuxtools.lttng.tests/src/org/eclipse/linuxtools/lttng/tests/state/handlers/after/StateAfterUpdateHandlers.java and StateBeforeUpdateHandlers.java imply that LTT_EVENT_FUNCTION_EXIT has no LTT_FIELD_CALL_SITE field. Is this true? Mind you, both of these test units also fail to mention LTT_EVENT_PAGE_FAULT_ENTRY, _PAGE_FAULT_EXIT, _PAGE_FAULT_NOSEM_ENTRY, _PAGE_FAULT_NOSEM_EXIT and _KPROBE... The following observations are not addressed by the patch, and are left as suggestions. Maybe they should be split off into a separate bug or bugs. The intent of StateStrings seems to be to provide essentially immutable collections of strings and enumerations designated by those strings (Why on Earth is Events.setParent public?); however, because the hash maps and string arrays are publicly accessible (e.g. private final String[] syscall_names is exposed through public String[] getSyscallNames() { return syscall_names; }), their contents (and hence the mapping) could conceivably be readily modified by, for instance, a malicious plug-in. The entire implementation seems highly insecure from this point of view. One way to secure the hash maps and string arrays would be to have the accessor methods return clones of the private objects instead of the objects themselves. The identifiers and the String names of various things in the StateStrings object are inconsistent, which prevents programmers from developing simple mnemonics; they'll have instead to rely on Eclipse's code completion facilities. Refactoring would fix this. The inconsistencies are listed below. The Enum identifiers are sometimes plural (Channels, Events, Fields, GenStates), sometimes singular (CpuMode, IRQMode, SoftIRQMode, BdevMode, TrapMode, ProcessType, ProcessStatus, ExecutionMode, ExecutionSubMode). Suggestion: Change CpuMode, IRQMode, SoftIRQMode, BdevMode, TrapMode, ProcessType, ProcessStatus, ExecutionMode, and ExecutionSubMode to CpuModes, IRQModes, SoftIRQModes, BdevModes, TrapModes, ProcessTypes, ProcessStatuses, ExecutionModes, and ExecutionSubModes, respectively. Soft IRQs are sometimes named SOFTIRQ, sometimes SOFT_IRQ, with no consistent pattern: within StateStrings.Events we have LTT_EVENT_SOFT_IRQ_RAISE, LTT_EVENT_SOFT_IRQ_ENTRY and LTT_EVENT_SOFT_IRQ_EXIT but also LTT_EVENT_SOFTIRQ_VEC. getInName returns "softirq" in all cases except for LTTV_STATE_RESOURCE_SOFT_IRQS which yields "soft irq [...]". Suggestions: Change LTT_CHANNEL_SOFTIRQ_STATE into LTT_CHANNEL_SOFT_IRQ_STATE Change LTT_EVENT_SOFTIRQ_VEC into LTT_EVENT_SOFT_IRQ_VEC Change LTTV_STATE_RESOURCE_SOFT_IRQS("soft irq resource states") into LTTV_STATE_RESOURCE_SOFT_IRQS("softirq resource states") The method String getInName() of each enum behaves inconsistently: most of the time it is the lower case version of the enum identifier (minus the common prefix); for instance, LTT_CHANNEL_FD_STATE yields "fd_state". However, one notes the following departures from this expectancy: * Events: *SOFT_IRQ* yields "*softirq*", *REQUEST* yields "_blk_request*" [note the "_" prefix], and *LIST_INTERRUPT yields "interrupt". * ExecutionMode: *SOFT_IRQ yields "SOFTIRQ", and the inNames are in upper case. * ExecutionSubMode: the inNames are in upper case. * Fields:*SOFT_IRQ* yields "*softirq*" and *OPERATION yields "direction". * GenStates: *HOOKS yields "saved state hooks", *RESOURCE_CPUS yields "cpu count", *RESOURCE_IRQS/SOFT_IRQS/TRAPS/BLKDEVS yield "irq/soft irq/trap/blkdevs resource states" [note how the BLKDEVS/blkdevs should have been BLKDEVS/blkdev]. Also, all members replace the expected "_" with " " (except for RUNNING_PROCESS and TRACE_STATE_USE_COUNT). * ProcessStatus: the inNames are in upper case. * ProcessType: the inNames are in upper case. Suggestions: * Pick one of "softirq" and "soft_irq" (in either case variation) and stick to it throughout. * Rename LTT_EVENT_REQUEST_ISSUE/_COMPLETE as LTT_EVENT_BLKDEV_REQUEST_ISSUE/_COMPLETE and use the "blkdev_request_issue"/"blkdev_request_complete" names accordingly. * Change LTT_EVENT_LIST_INTERRUPT("interrupt") into LTT_EVENT_LIST_INTERRUPT("list_interrupt") * Return the names of the ExecutionModes, ExecutionSubModes, ProcessStatuses, and ProcessTypes in lower case. * Change LTT_FIELD_OPERATION("direction") into LTT_FIELD_OPERATION("operation") * Change LTTV_STATE_SAVED_STATES_TIME("saved states time") into LTTV_STATE_SAVED_STATES_TIME("saved_states_time") * Change LTTV_STATE_HOOKS("saved state hooks") into LTTV_STATE_SAVED_STATE_HOOKS("saved_state_hooks") * Change LTTV_STATE_RESOURCE_CPUS("cpu count") into LTTV_STATE_CPU_COUNT("cpu_count") * Change LTTV_STATE_RESOURCE_IRQS("irq resource states") into LTTV_STATE_IRQ_RESOURCE_STATES("irq_resource_states") * Change LTTV_STATE_RESOURCE_SOFT_IRQS("soft irq resource states") into LTTV_STATE_SOFT_IRQ_RESOURCE_STATES("soft_irq_resource_states") * Change LTTV_STATE_RESOURCE_TRAPS("trap resource states") into LTTV_STATE_TRAP_RESOURCE_STATES("trap_resource_states") * Change LTTV_STATE_RESOURCE_BLKDEVS("blkdevs resource states") into LTTV_STATE_BLKDEV_RESOURCE_STATES("blkdev_resource_states")
Thanks for the patch, Daniel! I thought I should let you know that patches in excess of 250 lines must go through Eclipse legal review. Francois can open a "contribution questionnaire" (aka CQ) at portal.eclipse.org to start this process. It will help things along if you can state here that you are the sole author of this work and have permission from your employer (if they had anything to do with it) to contribute it under the terms of the EPL. Thanks again for the patch and looking forward to working with you.
(In reply to comment #1) > Thanks for the patch, Daniel! I thought I should let you know that patches in > excess of 250 lines must go through Eclipse legal review. Francois can open a > "contribution questionnaire" (aka CQ) at portal.eclipse.org to start this > process. It will help things along if you can state here that you are the sole > author of this work and have permission from your employer (if they had > anything to do with it) to contribute it under the terms of the EPL. > > Thanks again for the patch and looking forward to working with you. Most of the 250-lines size is due to simply moving things around and adding comments, but I'll note this legal restriction for the future. For now, yes, I am the sole author of this work and have permission from my employer to contribute it under the terms of the EPL. Let's forge ahead!
Note that if a patch author is empoyed at an Eclipse Foundation member company and the committer is employed at the same company, the 250 line restriction does not apply.
Hi, I will open the CQ as soon as I get back. Thanks for the contribution Daniel. Regards, /fc
Opened CQ 5526 (https://dev.eclipse.org/ipzilla/show_bug.cgi?id=5526)
... which was approved.
Actually, the CQ is not yet approved. You must mark it as requesting "parallel IP" and then the legal team will mark it with the "checkintocvs" flag at which point you can check it in. For more details, please read: http://wiki.eclipse.org/Development_Resources/HOWTO/Parallel_IP_Process and http://www.eclipse.org/legal/EclipseLegalProcessPoster.pdf Thanks!
Daniel, We are going through the IP process for this patch but it might yet take a little while to complete. I already know that I won't commit some of the proposed changes, probably enough to make the patch < 250 lines. However, I would like to see how long the process will take so I will let it run. The parts that I find problematic are the following: - We don't put "//if", "// for", "// <fn name>", etc... at the end of our control structures. Java style. - In the enums, the "/* */" are used as comment placeholders. They should stay as is unless someone feels like properly documenting them (i.e. what they mean). - In associate(), it can be argued that highlighting that an event triggers a state transition (stateTransition = true) could be quite valuable to the casual reader (I believe this was the original author's intent). I think it would be better to leave the default state transition to False (for a trivial performance cost). Note that the re-organization of events does add some clarity to the code. And the missing LTT_EVENT_KPROBE event was a nice find. Could you submit a new patch with these into consideration? If the patch is small enough, it can probably make it for 0.8.1. Thanks.
(In reply to comment #8) > - We don't put "//if", "// for", "// <fn name>", etc... at the end of our > control structures. Java style. Fixed. > - In the enums, the "/* */" are used as comment placeholders. They should stay > as is unless someone feels like properly documenting them (i.e. what they > mean). I've put them back in and tried to fill them in as much as possible. My understanding will need to be checked as I'm bound to get some things wrong. > - In associate(), it can be argued that highlighting that an event triggers a > state transition (stateTransition = true) could be quite valuable to the casual > reader (I believe this was the original author's intent). I think it would be > better to leave the default state transition to False (for a trivial > performance cost). Fixed. > Note that the re-organization of events does add some clarity to the code. And > the missing LTT_EVENT_KPROBE event was a nice find. I currently have this event as follows: // KPROBE LTT_EVENT_KPROBE.setParent(Channels.LTT_CHANNEL_KERNEL); LTT_EVENT_KPROBE.getChildren().add(Fields.LTT_FIELD_IP); LTT_EVENT_KPROBE.stateTransition = true; Is this correct? > Could you submit a new patch with these into consideration? If the patch is > small enough, it can probably make it for 0.8.1. Once I have my KPROBE answer, I'll submit the updated patch (143 suppressions, 142 additions, down from 338 and 295...Is the patch size the raw line count or the greater of the number of lines suppressed and added?).
Created attachment 203183 [details] Patch line counter Here's the script I'm using to count lines in a patch. It could be further refined but, as it stands now, it returns an upper bound on the number of lines in a patch. If, by Thursday, you can get this script to say that your patch is under 250 lines, submit the patch and I should be able to add it in time for 0.8.1 (else, it will end up in 0.9). Sorry for the hassle. Best Regards, /fc
(In reply to comment #9) > I currently have this event as follows: > > // KPROBE > LTT_EVENT_KPROBE.setParent(Channels.LTT_CHANNEL_KERNEL); > LTT_EVENT_KPROBE.getChildren().add(Fields.LTT_FIELD_IP); > LTT_EVENT_KPROBE.stateTransition = true; > > Is this correct? Very good. > Once I have my KPROBE answer, I'll submit the updated patch (143 suppressions, > 142 additions, down from 338 and 295...Is the patch size the raw line count or > the greater of the number of lines suppressed and added?). Lines truly added only (this could be a matter of debate with my fearless project leader :) From your numbers, the patch should be 142 lines.
Created attachment 203268 [details] Various fixes and improvements to StateStrings.java lcp says it's 130 lines. I recounted and I come up with 134 lines. I'll fool around and try to figure out which 4 lines were skipped. Note that "lcp -e whatever" is broken. I'll try to fix that also.
Created attachment 203285 [details] Patch line counter script Here is an improved lcp that handles its command-line options correctly, as well as handling a couple more error conditions.
Comment on attachment 203268 [details] Various fixes and improvements to StateStrings.java Patch committed. Thanks Daniel.
Commit ID: a08e87c902396b0c2a8f8857dd3ad5f513bdcbd4
(In reply to comment #12) > lcp says it's 130 lines. I recounted and I come up with 134 lines. I'll fool > around and try to figure out which 4 lines were skipped. For the record, no lines were skipped, it's just that all lines that consist of just an asterisk (*), closing curly bracket (}) or asterisk-slash (*/) are counted as "empty". Personally, I'd count as empty only those lines that truly consist of just blanks and a carriage return, but that's no skin off my nose.
Forgot to push this fix on the 0.8 branch :-( Sorry Daniel. It is in HEAD and will be in 0.9.
Delivered in 0.9.0
