apply most patches in chw's repository

[mxmanghi]

I applied to the current FA code for tclcurl the changes made in androwish's repository (https://www.androwish.org/index.html/tktview/b7f1f6c72d7026ef04b7e77b687de25a7cc836d7). I also reworked the curlWriteProcInvoke function that caused Tcl to segfault (!) when the call back definition was made by multiple words, like in the case where the callback is implemented by an object method.

[resuna]

Why was this closed?

[mxmanghi]

my mistake, I'm going to reopen it

…

-- M

On 8/13/24 16:37, Peter da Silva wrote: Why was this closed? — Reply to this email directly, view it on GitHub <https://github.com/ flightaware/tclcurl-fa#26#issuecomment-2286420731>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ AACAF5OF4TOCMDTOWNQBTMDZRIK3HAVCNFSM6AAAAABMOAED2OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOBWGQZDANZTGE>. You are receiving this because you modified the open/close state.Message ID: ***@***.***>

[resuna]

Thank you!

[resuna]

Looks like it would benefit from having enums matching the method/option lists in tclcurl.h, it would make tclcurl.c easier to follow. :)

[bovine]

Do you have a test case that can reproduce the crash?

[mxmanghi]

Yes definitely

…

-- M

On 8/13/24 16:44, Peter da Silva wrote: Looks like it would benefit from having enums matching the method/option lists in tclcurl.h, it would make tclcurl.c easier to follow. :) — Reply to this email directly, view it on GitHub <https://github.com/ flightaware/tclcurl-fa#26#issuecomment-2286436612>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ AACAF5OJFS7BUECNJNTXDQDZRILVTAVCNFSM6AAAAABMOAED2OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOBWGQZTMNRRGI>. You are receiving this because you modified the open/close state.Message ID: ***@***.***>

[mxmanghi]

Do you have a test case that can reproduce the crash?

I have 3 cases. They have been tested on Ubuntu Noble, but also with a custom installation of Tcl 8.6.14 with optimizer disabled and symbols loaded

1)
tclsh8.6 [~/Documents/tclcurl] package require TclCurl
7.22.0
tclsh8.6 [~/Documents/tclcurl] set curl [::curl::init]
curl1
tclsh8.6 [~/Documents/tclcurl] $curl configure -url https://centromajorana.it/ -header 1 -timeout 5000 -writeproc [namespace current]::invalidproc
tclsh8.6 [~/Documents/tclcurl] $curl perform
Segmentation fault (core dumped)

2)
tclsh8.6 [~/Documents/tclcurl] package require TclCurl
7.22.0
tclsh8.6 [~/Documents/tclcurl] set curl [::curl::init]
curl1
tclsh8.6 [~/Documents/tclcurl] ::oo::class create CurlHandler
::CurlHandler
tclsh8.6 [~/Documents/tclcurl] ::oo::define CurlHandler { method callback {s} { puts $s } }
tclsh8.6 [~/Documents/tclcurl] set ch [CurlHandler new]
tclsh8.6 [~/Documents/tclcurl] $curl configure -url https://centromajorana.it/ -header 1 -timeout 5000 -writeproc [list $ch callback]
tclsh8.6 [~/Documents/tclcurl] $curl perform
Segmentation fault (core dumped)

3)
tclsh8.6 [~/Documents/tclcurl] package require TclCurl
7.22.0
tclsh8.6 [~/Documents/tclcurl] proc callback {s} { puts [string length $s] }
tclsh8.6 [~/Documents/tclcurl] set curl [::curl::init]
curl1
tclsh8.6 [~/Documents/tclcurl] $curl configure -url https://centromajorana.it/ -header 1 -timeout 5000 -writeproc callback
tclsh8.6 [~/Documents/tclcurl] $curl perform
17
37
32
67
23
28
40
2
7940
1663
0
tclsh8.6 [~/Documents/tclcurl] 

[bovine]

If you sync your branch with our master, the broken CI builds should be fixed here too.

[bovine]

the Mac CI is legitimately failing on something involving SetoptBlob not being defined.

In the curlVersionInfo function, the tmp[7] variable can be removed since its only use was eliminated by the use of Tcl_ObjPrintf in the case 1 block.

There are a couple of other warnings as well.

[bovine]

Seems like fine improvements. I've suggested a couple of places where snprintf could be used during the handle generation.

@resuna might want to review the entire PR also.

[resuna]

Should this be built up as a list? What if pattern contains spaces?

[resuna]

Was anything actually changed here?

[mxmanghi]

No, it's easy to verify that the 2 option lists are just reorganized

[resuna]

Is there any possibility that multi_h won't be set? multi.c (and thus multi.h) seem to be incorporated unconditionally in the config files.

[mxmanghi]

I applied the last change suggested by Jeff. What is your policy for releasing? Is it worth a release? I'm making out of the whole stuff a package for Debian/Ubuntu, I can apply the new improvements as patches for the Debian packaging system but I rather prefer to release as is with a new patchlevel number

[bovine]

after we merge this PR, we might want to also get PR 22 (I suspect there will be some minor merge conflicts), and then tag a new release.

[bovine]

I've tagged v7.22.1 and left that other PR unmerged for now. It can be addressed in the future.

[mxmanghi]

Great! Thank you