#190 ophelp -X doesn't display event's unit mask type

closed-fixed
None
5
2011-08-12
2010-08-26
Roland Grunberg
No

oprofile-0.9.6-6.fc13.i686

It would be nice if "ophelp -X" displayed the unit mask type (mandatory, exclusive, bitmask) as an attribute in the <unit_masks> tag.

Discussion

  • 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.

     
    • assigned_to: nobody --> maynardj
     
  • 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.

     
  •  Patch, Signed-off-by: Roland Grunberg roland.grunberg@gmail.com
    
     
    Attachments
  • ophelp -X sample output

     
    Attachments
  • 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".

     
    • status: open --> open-fixed
     
    • status: open-fixed --> closed-fixed