OS-7560: libtopo's XML serialization code is broken and incomplete

Details

Issue Type:Bug
Priority:4 - Normal
Status:Resolved
Created at:2019-02-05T19:57:04.915Z
Updated at:2019-02-28T16:54:15.716Z

People

Created by:Former user
Reported by:Former user
Assigned to:Former user

Resolution

Fixed: A fix for this issue is checked into the tree and tested.
(Resolution Date: 2019-02-28T03:57:12.356Z)

Fix Versions

2019-02-28 Mind Grapes (Release Date: 2019-02-28)

Description

libtopo supports the ability to create a serialized representation of a topo snapshot as XML. However, the serialization code is incomplete in that it there are a number of node property types that aren't supported. For example, libtopo supports the following types for node properties (excerpted from libtopo.h):

typedef enum {
	TOPO_TYPE_INVALID = 0,
	TOPO_TYPE_BOOLEAN,	/* boolean */
	TOPO_TYPE_INT32,	/* int32_t */
	TOPO_TYPE_UINT32,	/* uint32_t */
	TOPO_TYPE_INT64,	/* int64_t */
	TOPO_TYPE_UINT64,	/* uint64_t */
	TOPO_TYPE_STRING,	/* const char* */
	TOPO_TYPE_TIME,		/* uint64_t */
	TOPO_TYPE_SIZE,		/* uint64_t */
	TOPO_TYPE_FMRI,		/* nvlist_t */
	TOPO_TYPE_INT32_ARRAY,	/* array of int32_t */
	TOPO_TYPE_UINT32_ARRAY,	/* array of uint32_t */
	TOPO_TYPE_INT64_ARRAY,	/* array of int64_t */
	TOPO_TYPE_UINT64_ARRAY,	/* array of uint64_t */
	TOPO_TYPE_STRING_ARRAY,	/* array of const char* */
	TOPO_TYPE_FMRI_ARRAY,	/* array of nvlist_t */
	TOPO_TYPE_DOUBLE	/* double */
} topo_type_t;

But the serialization code does not currently implement code to handle the following types (see txml_print_prop() in usr/src/lib/fm/topo/libtopo/common/topo_2xml.c):

	TOPO_TYPE_BOOLEAN,	/* boolean */
	TOPO_TYPE_TIME,		/* uint64_t */
	TOPO_TYPE_SIZE,		/* uint64_t */
	TOPO_TYPE_INT32_ARRAY,	/* array of int32_t */
	TOPO_TYPE_INT64_ARRAY,	/* array of int64_t */
	TOPO_TYPE_UINT64_ARRAY,	/* array of uint64_t */
	TOPO_TYPE_STRING_ARRAY,	/* array of const char* */
	TOPO_TYPE_FMRI_ARRAY,	/* array of nvlist_t */
	TOPO_TYPE_DOUBLE	/* double */

This ticket is to cover extending the current XML serialization code such that it can handle all of the supported property types.

Comments

Comment by Former user
Created at 2019-02-08T22:41:45.203Z
Updated at 2019-02-16T02:56:50.709Z

Note that the TOPO_TYPE_BOOLEAN, TOPO_TYPE_SIZE and TOPO_TYPE_TIME property types are defined in libtopo.h, but there is no support for them in the topo_prop_get|set code paths and there are no consumers of these types. Furthermore, the DTD for the XML representation of a topo snapshot does not allow for these types. I'm not sure what the original intention was for them. Anyways, we won't bother adding any special handling for these three types as part of this fix.


Comment by Former user
Created at 2019-02-16T03:10:32.703Z
Updated at 2019-02-16T03:11:48.381Z

Another significant issue with the current serialization code. It does not serialize any of the array property types correctly. Currently, array property values are serialized by placing the values of all of the array elements in the "value" attribute of the "propval" element, delimited by spaces - for example:

<propval name='assigned-addresses' type='uint32_array' value='0x83020010 0x0 0xfc000000 0x0 0x20000 0x83020018 0x0 0xfc020000 0x0 0x10000 0x81020020 0x0 0x1040 0x0 0x40' />

But this is not what libtopo's XML parser expects. The DTD specifies that the value for each array element should be specified in a "propitem" element that is a child of the "propval" element. For example:

         <propval name='entity-list' type='string_array' >
           <propitem value='Intrusion' />
           <propitem value='Pwr Consumption' />
           <propitem value='Inlet Temp' />
           <propitem value='Exhaust Temp' />
         </propval>

Below is the commit which updated the DTD and the XML parser to support array types - but the serialization code was never updated properly. I guess it's time to finish the job I started - only 10 years later :)

commit 88045cff0aae4ed8823cd0989168e8f56927f83e
Author: Robert Johnston <Robert.Johnston@Sun.COM>
Date:   Fri Jul 31 13:25:53 2009 -0700

    6839705 libtopo needs updates in order to cope with ILOM 3
    6840169 libtopo: topo xml schema and parsing code needs to be extended to support defining array propvals
    6840764 fmtopo can't print TOPO_TYPE_INT32_ARRAY and TOPO_TYPE_UINT64_ARRAY propvals
    6844530 dimm/cs serial propmethods in chip enumerator needlessly recompute IPMI entity name
    6836314 add support for sensor-transport module on ILOM-based X4450 platforms
    6844635 libtopo: pull chassis-specific xml out of i86pc-hc-topology.xml into seperate map
    6844639 libtopo: add DIMM serial to chip-select nodes on X4140/4240/4440
    6845699 libipmi: implementation of ipmi_sunoem_led_get/set interfaces needs to be updated for ILOM 3
    6677012 libtopo: small leaks on snapshot creation
    6535637 Add Severity level to payload of list.suspects event
    6850083 libtopo: need to add JEDEC id for Hyundai Electronics to jedec_tbl in the chip enumerator
    6844145 sys/bmc_intf.h should be delivered
    6855750 fmadm faulty will fail to expand message tokens that reference event payload
    6862378 libtopo: need to register TOPO_METH_SENSOR_FAILURE on ses nodes

Comment by Former user
Created at 2019-02-22T04:53:43.434Z
Updated at 2019-02-22T05:36:12.675Z

One more piece of brokenness. The serialization code always sets the "static" attribute to "true" for "node" elements. This has the effect of causing the parser to not create the node, if it doesn't exist. The static attribute is primarily there for when we want to use XML to override a property value on a topo node that was already created by an enumerator module. But in this case we're trying to serialize the whole topology in a fashion such that we could reconstitute it from the generated XML. In which case, we relly need it to create all the nodes becuase no enumerator modules will be running.


Comment by Former user
Created at 2019-02-23T04:53:32.164Z
Updated at 2019-02-26T21:15:00.262Z

Testing

To test this change I built a custom topo map that exercises all of the prop types affected by this change.

<?xml version="1.0"?>
<!DOCTYPE topology SYSTEM "/usr/share/lib/xml/dtd/topology.dtd.1">

<topology name='i86pc' scheme='hc'>

  <range name='motherboard' min='0' max='0'>
    <node instance='0'>
	<propgroup name='test_props' version='1'
	    name-stability='Private' data-stability='Private' >
	    <propval name='u64_arr' type='uint64_array' >
		<propitem value='0x1234567812345678' />
		<propitem value='10' />
		<propitem value='5' />
	    </propval>
	    <propval name='i64_arr' type='int64_array' >
		<propitem value='12345678123456' />
		<propitem value='-10' />
		<propitem value='5' />
		<propitem value='0xf' />
	    </propval>
	    <propval name='u32_arr' type='uint32_array' >
		<propitem value='0xdeadbeef' />
		<propitem value='10' />
		<propitem value='5' />
	    </propval>
	    <propval name='i32_arr' type='int32_array' >
		<propitem value='12345678' />
		<propitem value='-10' />
		<propitem value='5' />
		<propitem value='0xf' />
	    </propval>
	    <propval name='str_arr' type='string_array' >
		<propitem value='string one' />
		<propitem value='string two' />
	    </propval>
	    <propval name='fmri_arr' type='fmri_array' >
		<propitem value='hc:///motherboard=0' />
		<propitem value='hc:///motherboard=0/chip=0' />
	    </propval>
	    <propval name='int32' type='int32' value='-54321' />
	    <propval name='uint32' type='uint32' value='0xfeedface' />
	    <propval name='int64' type='int64' value='987654321' />
	    <propval name='uint64' type='uint64' value='0xbaddcafebaddcafe' />
	    <propval name='dbl' type='double' value='3.14159' />
	    <propval name='dbl2' type='double' value='0x1.921f9f01b866ep+1' />
	</propgroup>
    </node>

  </range>

</topology>

Then I verified that I could parse it, by loopback mounting it over the top of /usr/platform/i86pc/lib/fm/topo/maps/i86pc-legacy-hc-topology.xml and then dumping the topo snapshot with fmtopo. Afterwards, I serialized it (fmtopo -x) and then verified that libtopo could then parse the serialized output.

[root@smartos-vm2 /var/tmp]# df -kl|grep topo
/var/tmp/i86pc-legacy-hc-topology.xml     4267424      167915     4099509     4%    /usr/platform/i86pc/lib/fm/topo/maps/i86pc-legacy-hc-topology.xml

[root@smartos-vm2 /var/tmp]# ./fmtopo -V
TIME                 UUID
Feb 26 21:11:01 8dbed39b-ffa4-45ea-ff70-a165b9da2a25

hc://:product-id=VMware-Virtual-Platform:server-id=smartos-vm2:chassis-id=VMware-56-4d-dd-f6-d5-8e-e2-e3-55-df-84-99-6b-a3-b3-cd/motherboard=0
  group: protocol                       version: 1   stability: Private/Private
    resource          fmri      hc://:product-id=VMware-Virtual-Platform:server-id=smartos-vm2:chassis-id=VMware-56-4d-dd-f6-d5-8e-e2-e3-55-df-84-99-6b-a3-b3-cd/motherboard=0
    FRU               fmri      hc://:product-id=VMware-Virtual-Platform:server-id=smartos-vm2:chassis-id=VMware-56-4d-dd-f6-d5-8e-e2-e3-55-df-84-99-6b-a3-b3-cd/motherboard=0
  group: authority                      version: 1   stability: Private/Private
    product-id        string    VMware-Virtual-Platform
    chassis-id        string    VMware-56-4d-dd-f6-d5-8e-e2-e3-55-df-84-99-6b-a3-b3-cd
    server-id         string    smartos-vm2
  group: system                         version: 1   stability: Private/Private
    isa               string    i386
    machine           string    i86pc
  group: test_props                     version: 1   stability: Private/Private
    u64_arr           uint64[]  [ 1311768465173141112 10 5 ]
    i64_arr           int64[]   [ 12345678123456 -10 5 15 ]
    u32_arr           uint32[]  [ 3735928559 10 5 ]
    i32_arr           int32[]   [ 12345678 -10 5 15 ]
    str_arr           string[]  [ "string one" "string two" ]
    fmri_arr          fmri[]    [ "hc:///motherboard=0" "hc:///motherboard=0/chip=0" ]
    int32             int32     -54321
    uint32            uint32    0xfeedface
    int64             int64     987654321
    uint64            uint64    0xbaddcafebaddcafe
    dbl               double    3.141590
    dbl2              double    3.141590

[root@smartos-vm2 /var/tmp]# ./fmtopo -x > generated.xml 
[root@smartos-vm2 /var/tmp]# cat generated.xml 
<?xml version="1.0"?>
<!DOCTYPE topology SYSTEM "/usr/share/lib/xml/dtd/topology.dtd.1">
<!--
 This topology map file was generated on Feb 26 21:11:56 for smartos-vm2
<-->

<topology name='VMware-Virtual-Platform' scheme='hc' >
<range name='motherboard' min='0' max='0' >
<node instance='0' static='false' >
<propgroup name='protocol' name-stability='Private' data-stability='Private' version='1' >
<propval name='resource' type='fmri' value='hc://:product-id=VMware-Virtual-Platform:server-id=smartos-vm2:chassis-id=VMware-56-4d-dd-f6-d5-8e-e2-e3-55-df-84-99-6b-a3-b3-cd/motherboard=0' />
<propval name='FRU' type='fmri' value='hc://:product-id=VMware-Virtual-Platform:server-id=smartos-vm2:chassis-id=VMware-56-4d-dd-f6-d5-8e-e2-e3-55-df-84-99-6b-a3-b3-cd/motherboard=0' />
</propgroup>
<propgroup name='authority' name-stability='Private' data-stability='Private' version='1' >
<propval name='product-id' type='string' value='VMware-Virtual-Platform' />
<propval name='chassis-id' type='string' value='VMware-56-4d-dd-f6-d5-8e-e2-e3-55-df-84-99-6b-a3-b3-cd' />
<propval name='server-id' type='string' value='smartos-vm2' />
</propgroup>
<propgroup name='system' name-stability='Private' data-stability='Private' version='1' >
<propval name='isa' type='string' value='i386' />
<propval name='machine' type='string' value='i86pc' />
</propgroup>
<propgroup name='test_props' name-stability='Private' data-stability='Private' version='1' >
<propval name='u64_arr' type='uint64_array' >
<propitem value='0x1234567812345678' />
<propitem value='0xa' />
<propitem value='0x5' />
</propval>
<propval name='i64_arr' type='int64_array' >
<propitem value='12345678123456' />
<propitem value='-10' />
<propitem value='5' />
<propitem value='15' />
</propval>
<propval name='u32_arr' type='uint32_array' >
<propitem value='0xdeadbeef' />
<propitem value='0xa' />
<propitem value='0x5' />
</propval>
<propval name='i32_arr' type='int32_array' >
<propitem value='12345678' />
<propitem value='-10' />
<propitem value='5' />
<propitem value='15' />
</propval>
<propval name='str_arr' type='string_array' >
<propitem value='string one' />
<propitem value='string two' />
</propval>
<propval name='fmri_arr' type='fmri_array' >
<propitem value='hc:///motherboard=0' />
<propitem value='hc:///motherboard=0/chip=0' />
</propval>
<propval name='int32' type='int32' value='-54321' />
<propval name='uint32' type='uint32' value='0xfeedface' />
<propval name='int64' type='int64' value='987654321' />
<propval name='uint64' type='uint64' value='0xbaddcafebaddcafe' />
<propval name='dbl' type='double' value='0x1.921f9f01b866ep+1' />
<propval name='dbl2' type='double' value='0x1.921f9f01b866ep+1' />
</propgroup>
</node>
</range>
<range name='ses-enclosure' min='0' max='1024' >
</range>
</topology>
[root@smartos-vm2 /var/tmp]# cp -f generated.xml i86pc-legacy-hc-topology.xml
[root@smartos-vm2 /var/tmp]# ./fmtopo -V
TIME                 UUID
Feb 26 21:12:25 9ddb372d-c0d0-4979-fd84-dbd28e38b67f

hc://:product-id=VMware-Virtual-Platform:server-id=smartos-vm2:chassis-id=VMware-56-4d-dd-f6-d5-8e-e2-e3-55-df-84-99-6b-a3-b3-cd/motherboard=0
  group: protocol                       version: 1   stability: Private/Private
    resource          fmri      hc://:product-id=VMware-Virtual-Platform:server-id=smartos-vm2:chassis-id=VMware-56-4d-dd-f6-d5-8e-e2-e3-55-df-84-99-6b-a3-b3-cd/motherboard=0
    FRU               fmri      hc://:product-id=VMware-Virtual-Platform:server-id=smartos-vm2:chassis-id=VMware-56-4d-dd-f6-d5-8e-e2-e3-55-df-84-99-6b-a3-b3-cd/motherboard=0
  group: authority                      version: 1   stability: Private/Private
    product-id        string    VMware-Virtual-Platform
    chassis-id        string    VMware-56-4d-dd-f6-d5-8e-e2-e3-55-df-84-99-6b-a3-b3-cd
    server-id         string    smartos-vm2
  group: system                         version: 1   stability: Private/Private
    isa               string    i386
    machine           string    i86pc
  group: test_props                     version: 1   stability: Private/Private
    u64_arr           uint64[]  [ 1311768465173141112 10 5 ]
    i64_arr           int64[]   [ 12345678123456 -10 5 15 ]
    u32_arr           uint32[]  [ 3735928559 10 5 ]
    i32_arr           int32[]   [ 12345678 -10 5 15 ]
    str_arr           string[]  [ "string one" "string two" ]
    fmri_arr          fmri[]    [ "hc:///motherboard=0" "hc:///motherboard=0/chip=0" ]
    int32             int32     -54321
    uint32            uint32    0xfeedface
    int64             int64     987654321
    uint64            uint64    0xbaddcafebaddcafe
    dbl               double    3.141590
    dbl2              double    3.141590

I also booted a PI with this change on sky1 and verified that the topology was correctly generated.


Comment by Jira Bot
Created at 2019-02-28T03:57:10.605Z

illumos-joyent commit f2b29c149057da13a23c7c86492e7baf539e4b7f (branch master, by Rob Johnston)

OS-7560 libtopo's XML serialization code is broken and incomplete
OS-7609 fmtopo is missing code to handle properties of type TOPO_TYPE_FMRI_ARRAY
Reviewed by: Robert Mustacchi <rm@joyent.com>
Approved by: Jordan Hendricks <jordan.hendricks@joyent.com>


Comment by Jira Bot
Created at 2019-02-28T16:54:15.716Z

illumos-joyent commit 3abe99b08f51c708615fde3464d90ceab6f54837 (branch master, by Rob Johnston)

OS-7560 libtopo's XML serialization code is broken and incomplete (fix unused var)
OS-7609 fmtopo is missing code to handle properties of type TOPO_TYPE_FMRI_ARRAY (fix unused var)