Thanks for the patch submission. A few comments:
1. The patch gets a compile error when applied to current CVS (minor one line change needed).
2. I suggest changing the new unit mask attribute name from "type" to something else (e.g., "category"), since "type" is a reserved word in xml schema language. It may be OK to do, but it could tend to be confusing to some.
3. The doc/ophelp.xsd should be updated to include the new optional attribute.
Regarding comment #3, your patch motivated me to check our current ophelp schema doc and the ophelp XML instances our code creates using an XML validator, and unfortunately, I found some problems. I'll be posting a patch to fix those problems, which will include fixes to doc/ophelp.xsd, so please wait until that patch is committed to our CVS before you re-spin your patch to address my above comments. Oh, and when you do submit a new patch, could you also attach an example XML instance that I can run through the XML validator (the one I was using can be found at http://tools.decisionsoft.com/schemaValidate/\).
Thanks.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Roland, I just posted a patch to the oprofile-list that fixes some problems in ophelp.xsd and the generated XML output. Please pull the latest oprofile CVS, apply that patch, and then respin your patch, addressing my aforementioned review comments.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Roland, your patch looks good, with one caveat. I think, by rights, we should bump the schema version to "1.1" since the new attribute being added is required.
As you know, your patch is dependent on the patch that I posted to the list (mentioned below) to fix other unrelated schema issues with ophelp.xsd. I've not been able to get Richard Purdie's attention to review that patch. But as a user of ophelp's XML output, your review would be more than satisfactory. So please find that original posting of mine (Sept 8, subject "[Patch] Fix ophelp schema validation problems and xml generation error") and give it a thumbs up. Please also indicate that you are a consumer of ophelp XML output and mention that this patch is a pre-req to a patch you submitted for this bug. Once that's done, I'll commit both that patch and yours. Thanks.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Roland, I'd like to close out this bug and get your patch committed. But as I said in my September 30 comment to this bug . . .
"your patch is dependent on the patch that I posted to the
list (mentioned below) to fix other unrelated schema issues with
ophelp.xsd. I've not been able to get Richard Purdie's attention to review
that patch. But as a user of ophelp's XML output, your review would be
more than satisfactory. So please find that original posting of mine (Sept
8, subject "[Patch] Fix ophelp schema validation problems and xml
generation error") and give it a thumbs up. Please also indicate that you
are a consumer of ophelp XML output and mention that this patch is a
pre-req to a patch you submitted for this bug. Once that's done, I'll
commit both that patch and yours."
Can you send your review comments to that patch to the oprofile-list please? Thanks.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
The patch for this bug was committed to oprofile CVS on Oct 15, 2010. I neglected to update this bugzilla with that information until now. Changed "Resolution" field to "Fixed".
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Can you submit a patch? I'd be happy to review it.
Hi, the patch is was the attached file "unit_mask_type.patch".
It's small enough that I could have probably posted it here, so sorry for the confusion.
(hope this shows up correctly)
diff -ru oprofile-0.9.6/libop/op_xml_events.c oprofile-0.9.6-new/libop/op_xml_events.c
--- oprofile-0.9.6/libop/op_xml_events.c 2009-11-24 10:25:17.000000000 -0500
+++ oprofile-0.9.6-new/libop/op_xml_events.c 2010-08-26 14:30:26.000000000 -0400
@@ -80,6 +80,19 @@
open_xml_element(HELP_UNIT_MASKS, 1, buffer, MAX_BUFFER);
init_xml_int_attr(HELP_DEFAULT_MASK, event->unit->default_mask,
buffer, MAX_BUFFER);
+ char um_type[10];
+ switch (event->unit->unit_type_mask){
+ case utm_bitmask:
+ strncpy(um_type, "bitmask", sizeof(um_type));
+ break;
+ case utm_exclusive:
+ strncpy(um_type, "exclusive", sizeof(um_type));
+ break;
+ case utm_mandatory:
+ strncpy(um_type, "mandatory", sizeof(um_type));
+ break;
+ }
+ init_xml_str_attr(HELP_UNIT_MASKS_TYPE, um_type, buffer, MAX_BUFFER);
close_xml_element(NONE, 1, buffer, MAX_BUFFER);
for (i = 0; i < event->unit->num; i++) {
open_xml_element(HELP_UNIT_MASK, 1, buffer, MAX_BUFFER);
diff -ru oprofile-0.9.6/libop/op_xml_out.c oprofile-0.9.6-new/libop/op_xml_out.c
--- oprofile-0.9.6/libop/op_xml_out.c 2009-11-24 10:25:17.000000000 -0500
+++ oprofile-0.9.6-new/libop/op_xml_out.c 2010-08-26 12:21:23.000000000 -0400
@@ -80,6 +80,7 @@
"ext",
"unit_masks",
"default",
+ "type",
"unit_mask",
"mask",
"desc"
diff -ru oprofile-0.9.6/libop/op_xml_out.h oprofile-0.9.6-new/libop/op_xml_out.h
--- oprofile-0.9.6/libop/op_xml_out.h 2009-11-24 10:25:17.000000000 -0500
+++ oprofile-0.9.6-new/libop/op_xml_out.h 2010-08-26 12:19:53.000000000 -0400
@@ -54,6 +54,7 @@
HELP_EXT,
HELP_UNIT_MASKS,
HELP_DEFAULT_MASK,
+ HELP_UNIT_MASKS_TYPE,
HELP_UNIT_MASK,
HELP_UNIT_MASK_VALUE,
HELP_UNIT_MASK_DESC
Resubmitted patch as attachment to conform with contribution guidelines.
Thanks for the patch submission. A few comments:
1. The patch gets a compile error when applied to current CVS (minor one line change needed).
2. I suggest changing the new unit mask attribute name from "type" to something else (e.g., "category"), since "type" is a reserved word in xml schema language. It may be OK to do, but it could tend to be confusing to some.
3. The doc/ophelp.xsd should be updated to include the new optional attribute.
Regarding comment #3, your patch motivated me to check our current ophelp schema doc and the ophelp XML instances our code creates using an XML validator, and unfortunately, I found some problems. I'll be posting a patch to fix those problems, which will include fixes to doc/ophelp.xsd, so please wait until that patch is committed to our CVS before you re-spin your patch to address my above comments. Oh, and when you do submit a new patch, could you also attach an example XML instance that I can run through the XML validator (the one I was using can be found at http://tools.decisionsoft.com/schemaValidate/\).
Thanks.
Roland, I just posted a patch to the oprofile-list that fixes some problems in ophelp.xsd and the generated XML output. Please pull the latest oprofile CVS, apply that patch, and then respin your patch, addressing my aforementioned review comments.
Patch is posted, and applied on top of the one mentioned. xml file included as well.
Roland, your patch looks good, with one caveat. I think, by rights, we should bump the schema version to "1.1" since the new attribute being added is required.
As you know, your patch is dependent on the patch that I posted to the list (mentioned below) to fix other unrelated schema issues with ophelp.xsd. I've not been able to get Richard Purdie's attention to review that patch. But as a user of ophelp's XML output, your review would be more than satisfactory. So please find that original posting of mine (Sept 8, subject "[Patch] Fix ophelp schema validation problems and xml generation error") and give it a thumbs up. Please also indicate that you are a consumer of ophelp XML output and mention that this patch is a pre-req to a patch you submitted for this bug. Once that's done, I'll commit both that patch and yours. Thanks.
ophelp -X sample output
Minor change to patch. (bump up schema version to 1.1)
Updated patch looks fine. Thanks.
Roland, I'd like to close out this bug and get your patch committed. But as I said in my September 30 comment to this bug . . .
"your patch is dependent on the patch that I posted to the
list (mentioned below) to fix other unrelated schema issues with
ophelp.xsd. I've not been able to get Richard Purdie's attention to review
that patch. But as a user of ophelp's XML output, your review would be
more than satisfactory. So please find that original posting of mine (Sept
8, subject "[Patch] Fix ophelp schema validation problems and xml
generation error") and give it a thumbs up. Please also indicate that you
are a consumer of ophelp XML output and mention that this patch is a
pre-req to a patch you submitted for this bug. Once that's done, I'll
commit both that patch and yours."
Can you send your review comments to that patch to the oprofile-list please? Thanks.
The patch for this bug was committed to oprofile CVS on Oct 15, 2010. I neglected to update this bugzilla with that information until now. Changed "Resolution" field to "Fixed".