985306 - [NFC] Parse route.xml with expat library

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

For the development of NFC, the NFC daemon needs libxml2 to parse XML file to retrieve the configuration (route.xml) stored on the device.
BTW, the configuration(route.xml) is used to configure the routing of secure element.

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

The route.xml seems to be only used by NXP now.

Comment 2

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Just a stupid question: why libxml2? Last time I looked at it (on the desktop), it looked like a bloat-fest with a horrible API and several dependencies. Why not expat instead? It's small, lightweight and self-contained (again on the desktop).

Comment 3

[Thomas Zimmermann [:tzimmermann] [:tdz]]

And expat is already part of the build.

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Thanks for the info, Thomas.
libxml2 is used only by NXP, I don't know the reason either.

But we will also check if we can use expat instead.

Comment 5

[[:fabrice] Fabrice Desré]

If they use the SAX api moving to expat should be easy. If they use the tree-based one that will be more work. I don't remember libxml2 having tons of dependencies though:
Depends: libc6 (>= 2.15), liblzma5 (>= 5.1.1alpha+20120614), zlib1g (>= 1:1.2.3.3)

Comment 6

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Maybe I'm mistaken about the dependencies. It's been a while since I investigated different XML libraries.

But I'm a bit confused now. The initial report sounds like nfcd requires libxml2, but in comment #4 it sounds like the NXP driver requires it. The latter would be unfortunate, of course.

Comment 7

[[:fabrice] Fabrice Desré]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #6)
> Maybe I'm mistaken about the dependencies. It's been a while since I
> investigated different XML libraries.

One monster in this space that I can think of is the c++ version of Xerces ;)

Comment 9

[Dimi Lee [:dimi]]

NCI will also require the xml parser to parse route.xml if vendor has provided it.
Route.xml is used to decide the routing table for secure element.

I will check if we can change libxml2 to expat

Comment 10

[Dimi Lee [:dimi]]

Attachment: Parse route.xml with expat library

The network is too slow now so i cannot update to github, use git patch for review instead.

Currently route.xml doesn't push to flame so I verified by pushing manually.
This patch only parse the route.xml if exists and will not apply routing rule.

Apply routing rule will be the following task.

Comment 11

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8463290 [details] [diff] [review]
Parse route.xml with expat library

Review of attachment 8463290 [details] [diff] [review]:
-----------------------------------------------------------------

::: src/nci/RouteDataSet.cpp
@@ +35,5 @@
> +  const char* switchOff = "SwitchOff";
> +  const char* batteryOff = "BatteryOff";
> +  RouteDataForProtocol* data = new RouteDataForProtocol;
> +
> +  for(int i = 0; attribute[i]; i += 2) {

space after for.

@@ +50,5 @@
> +        data->mProtocol = NFA_PROTOCOL_MASK_T3T;
> +      } else if (strcmp(value, "IsoDep") == 0) {
> +        data->mProtocol = NFA_PROTOCOL_MASK_ISO_DEP;
> +      }
> +

extra line. same below

@@ +111,5 @@
> +
> +void RouteDataSet::xmlStartElement(void *data, const char *element, const char **attribute)
> +{
> +  RouteDataSet* route = reinterpret_cast<RouteDataSet*>(data);
> +  if (!route)

nit, {}

@@ +114,5 @@
> +  RouteDataSet* route = reinterpret_cast<RouteDataSet*>(data);
> +  if (!route)
> +    return;
> +
> +  if (strcmp(element, "Route") == 0) {

const strings.

@@ +115,5 @@
> +  if (!route)
> +    return;
> +
> +  if (strcmp(element, "Route") == 0) {
> +    for(uint32_t i = 0; attribute[i]; i += 2) {

nit, space after 'for'

@@ +128,5 @@
> +        }
> +      }
> +    }
> +  } else if (strcmp(element, "Tech") == 0) {
> +    RouteDataSet::importTechnologyRoute(attribute, route->xmlGetRouteDB());

These two import... functions shouldn't be static.

@@ +129,5 @@
> +      }
> +    }
> +  } else if (strcmp(element, "Tech") == 0) {
> +    RouteDataSet::importTechnologyRoute(attribute, route->xmlGetRouteDB());
> +  } else if (strcmp(element, "Protocol") == 0) {

Proto

@@ +191,5 @@
> +  XML_SetElementHandler(parser,
> +                        RouteDataSet::xmlStartElement,
> +                        RouteDataSet::xmlEndElement);
> +
> +  while(true) {

space after while

::: src/nci/RouteDataSet.h
@@ +32,5 @@
> +  RouteType mRouteType;
> +
> +protected:
> +  RouteData(RouteType routeType)
> +  : mRouteType(routeType)

indent, same for below

Comment 12

[Dimi Lee [:dimi]]

Attachment: pull request to mozilla-b2g/platform_system_nfcd v1

Comment 13

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8464431 [details] [review]
pull request to mozilla-b2g/platform_system_nfcd v1

see github for nits.

Comment 14

[Dimi Lee [:dimi]]

Attachment: pull request to mozilla-b2g/platform_system_nfcd v2

Address nits

Comment 15

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://github.com/mozilla-b2g/platform_system_nfcd/commit/1adbb04024c6181f1089f3a12ee46f26663ea1db