Menu
▾
▴
Re: [pupnp-devel] [PATCH] rewrite soap_device.c
|
From: Peng <how...@gm...> - 2014-03-29 03:50:16
|
Hi Marcelo, On 02/21/2014 01:08 AM, Marcelo Roberto Jimenez wrote: > Hi Peng, > > I am terribly sorry for the long time I took to review your patches. > > I liked your script tests a lot! If you authorize me, I will commit > them to the project. No problem. > > The only (minor) issue I had was a message in soap_no_soapaction.sh: > ./soap_no_soapaction.sh: line 53: PIPESTATUS[2]: unbound variable > It does not appear on my machine. I want to check the exit status of the third command of the pipeline. > Regards, > Marcelo. > > > > On Wed, Jan 15, 2014 at 10:24 AM, Peng <how...@gm... > <mailto:how...@gm...>> wrote: > > > On 01/15/2014 07:48 PM, Marcelo Roberto Jimenez wrote: >> Hi Peng, >> >> Thank you very much for your patches, I will take a look at them >> as soon as possible. >> >> Just to get things straight, 0001-rewrite-soap_device.c.patch is >> the same patch you have previously sent, just cherry picked to >> the master branch, and 0002-Fix-memory-leaks-caused-by-DOMString >> only applies to master. Are these statements correct? > Yes. And the memory leaks only exists for the master branch. > Please review these patches, and comment if necessary. I will > modify them according to comments. > >> >> Again, thank you for your work. >> >> Regards, >> Marcelo. >> >> >> >> On Wed, Jan 15, 2014 at 7:20 AM, Peng <how...@gm... >> <mailto:how...@gm...>> wrote: >> >> I cherry-picked it to the master branch (see the first patch). >> >> When doing unit test (with test cases attached in the >> previous mail) for soap device, I found several memory leaks: >> ==19438== 2 bytes in 1 blocks are definitely lost in loss >> record 1 of 4 >> ==19438== at 0x40299D8: malloc (in >> /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so) >> ==19438== by 0x414B3B0: strdup (strdup.c:42) >> ==19438== by 0x407E6CE: ixmlCloneDOMString (ixml.c:458) >> ==19438== by 0x405D710: UpnpStateVarRequest_set_CurrentVal >> (TemplateSource.h:334) >> ==19438== by 0x804B583: TvDeviceHandleGetVarRequest >> (tv_device.c:373) >> ==19438== by 0x804B8EF: TvDeviceCallbackEventHandler >> (tv_device.c:1272) >> ==19438== by 0x4: ??? >> ==19438== >> ==19438== 2 bytes in 1 blocks are definitely lost in loss >> record 2 of 4 >> ==19438== at 0x40299D8: malloc (in >> /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so) >> ==19438== by 0x414B3B0: strdup (strdup.c:42) >> ==19438== by 0x407E6CE: ixmlCloneDOMString (ixml.c:458) >> ==19438== by 0x405D710: UpnpStateVarRequest_set_CurrentVal >> (TemplateSource.h:334) >> ==19438== by 0x804B583: TvDeviceHandleGetVarRequest >> (tv_device.c:373) >> ==19438== by 0x804B8EF: TvDeviceCallbackEventHandler >> (tv_device.c:1272) >> ==19438== by 0xA: ??? >> ==19438== >> ==19438== 18 bytes in 2 blocks are definitely lost in loss >> record 3 of 4 >> ==19438== at 0x40299D8: malloc (in >> /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so) >> ==19438== by 0x414B3B0: strdup (strdup.c:42) >> ==19438== by 0x407E6CE: ixmlCloneDOMString (ixml.c:458) >> ==19438== by 0x405C8B0: UpnpFileInfo_set_ContentType >> (TemplateSource.h:334) >> ==19438== by 0x403E97F: get_file_info (webserver.c:347) >> ==19438== by 0x4051D38: web_server_callback (webserver.c:1173) >> ==19438== by 0x1: ??? >> ==19438== >> >> These leaks also exist for previous version, the second patch >> fixed these. >> >> >> On 01/03/2014 08:52 PM, Peng wrote: >> >> Hi, everyone. >> >> I just re-write soap_device.c with the following aims in >> mind: >> >> 1) separate HTTP handling from SOAP handling >> HTTP and SOAP handling are mixed together, and some HTTP >> errors are treated (inappropriately) as SOAP error (see 3 >> below). Mixed handling makes fixing 3) difficult. >> >> 2) remove repeated (and unnecessary) validity checks, >> each check should be performed *exactly once* >> For example, SOAPACTION hdr is checked in both >> get_request_type and check_soap_action_header. Another >> such example is the xml entity (Envelope). These checks >> are both inefficient and unnecessary, and are thus removed. >> >> 3) fix HTTP status code per UPnP spec, SOAP spec and RFC 2774 >> UPnP spec states that if the CONTENT-TYPE header >> specifies an unsupported value (other then "text/xml") >> the device must return an HTTP status code "415 >> Unsupported Media Type". RFC 2774 states that a server >> receiving a mandatory request including the "M-" method >> name prefix without any mandatory extension declarations >> to follow MUST return a 510 (Not Extended) response. >> Also, I try to follow SOAP 1.2's spirit, which has a >> complete description of the finite state machine of the >> server, since SOAP 1.1 is quite vague on this. >> >> As a side effect, the patch fixed the following over-read >> error, which was detected by valgrind when running unit >> tests against the previous version(see attached testing >> scripts): >> ==10174== Thread : >> ==10174== Invalid read of size 4 >> ==10174== at 0x40402DC: check_soap_action_header >> (soap_device.c:444) >> ==10174== by 0x404058D: get_device_info >> (soap_device.c:527) >> ==10174== by 0x4040EC1: handle_invoke_action >> (soap_device.c:779) >> ==10174== by 0x4041518: soap_device_callback >> (soap_device.c:870) >> ==10174== by 0x40429CA: handle_request (miniserver.c:157) >> ==10174== by 0xBBA10001: ??? >> >> If cherry-picking this patch were time-consuming, I would >> prepare another patch for the master branch. >> >> PS: Happy new years! >> >> >> -- >> Peng >> >> >> ------------------------------------------------------------------------------ >> CenturyLink Cloud: The Leader in Enterprise Cloud Services. >> Learn Why More Businesses Are Choosing CenturyLink Cloud For >> Critical Workloads, Development Environments & Everything In >> Between. >> Get a Quote or Start a Free Trial Today. >> http://pubads.g.doubleclick.net/gampad/clk?id=119420431&iu=/4140/ostg.clktrk >> _______________________________________________ >> pupnp-devel mailing list >> pup...@li... >> <mailto:pup...@li...> >> https://lists.sourceforge.net/lists/listinfo/pupnp-devel >> >> >> >> >> ------------------------------------------------------------------------------ >> CenturyLink Cloud: The Leader in Enterprise Cloud Services. >> Learn Why More Businesses Are Choosing CenturyLink Cloud For >> Critical Workloads, Development Environments & Everything In Between. >> Get a Quote or Start a Free Trial Today. >> http://pubads.g.doubleclick.net/gampad/clk?id=119420431&iu=/4140/ostg.clktrk >> >> >> _______________________________________________ >> pupnp-devel mailing list >> pup...@li... <mailto:pup...@li...> >> https://lists.sourceforge.net/lists/listinfo/pupnp-devel > > -- > Peng > > > ------------------------------------------------------------------------------ > CenturyLink Cloud: The Leader in Enterprise Cloud Services. > Learn Why More Businesses Are Choosing CenturyLink Cloud For > Critical Workloads, Development Environments & Everything In Between. > Get a Quote or Start a Free Trial Today. > http://pubads.g.doubleclick.net/gampad/clk?id=119420431&iu=/4140/ostg.clktrk > _______________________________________________ > pupnp-devel mailing list > pup...@li... > <mailto:pup...@li...> > https://lists.sourceforge.net/lists/listinfo/pupnp-devel > > > > > ------------------------------------------------------------------------------ > Managing the Performance of Cloud-Based Applications > Take advantage of what the Cloud has to offer - Avoid Common Pitfalls. > Read the Whitepaper. > http://pubads.g.doubleclick.net/gampad/clk?id=121054471&iu=/4140/ostg.clktrk > > > _______________________________________________ > pupnp-devel mailing list > pup...@li... > https://lists.sourceforge.net/lists/listinfo/pupnp-devel -- Peng |