Closed Bug 985306 Opened 7 years ago Closed 7 years ago

[NFC] Parse route.xml with expat library

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S1 (1aug)

People

(Reporter: allstars.chh, Assigned: dlee)

References

Details

(Whiteboard: [p=2])

Attachments

(1 file, 2 obsolete files)

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.
The route.xml seems to be only used by NXP now.
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).
And expat is already part of the build.
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.
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)
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.
(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 ;)
Duplicate of this bug: 1006460
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
Status: NEW → ASSIGNED
Assignee: nobody → dlee
Summary: Add libxml2 into gonk-jb and gonk-kk → [NFC] Parsing route.xml with expat library
Summary: [NFC] Parsing route.xml with expat library → [NFC] 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.
Attachment #8463290 - Flags: review?(allstars.chh)
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
Attachment #8463290 - Flags: review?(allstars.chh)
Attachment #8463290 - Attachment is obsolete: true
Attachment #8464431 - Flags: review?(allstars.chh)
Whiteboard: [p=2]
Target Milestone: --- → 2.1 S1 (1aug)
Comment on attachment 8464431 [details] [review]
pull request to mozilla-b2g/platform_system_nfcd v1

see github for nits.
Attachment #8464431 - Flags: review?(allstars.chh) → review+
Address nits
Attachment #8464431 - Attachment is obsolete: true
Attachment #8464445 - Flags: review+
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.