[pacman-dev] [PATCH 1/2] dload: unhook error buffer after transfer finishes

newer
[pacman-dev] [PATCH] pactree: Add...

Dave Reisner

10 Oct 2011 10 Oct '11

3:35 a.m.

Similar to what we did in edd9ed6a, disconnect the relationship with our stack allocated error buffer from the curl handle. Just as an FTP connection might have some network chatter on teardown causing the progress callback to be triggered, we might also hit an error condition that causes curl to write to our (now out of scope) error buffer. I'm unable to reproduce FS#26327, but I have a suspicion that this should fix it. Signed-off-by: Dave Reisner --- lib/libalpm/dload.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index 33824be..e848b30 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -377,8 +377,11 @@ static int curl_download_internal(struct dload_payload *payload, /* perform transfer */ payload->curlerr = curl_easy_perform(curl); - /* immediately unhook the progress callback */ + /* disconnect relationships from the curl handle for things that might go out + * of scope, but could still be touched on connection teardown. This really + * only applies to FTP transfers. See FS#26327 for an example. */ curl_easy_setopt(curl, CURLOPT_NOPROGRESS, 1L); + curl_easy_setopt(curl, CURLOPT_ERRORBUFFER, (char *)0); /* was it a success? */ switch(payload->curlerr) { -- 1.7.7

Show replies by date

Dave Reisner

10 Oct 10 Oct

3:35 a.m.

New subject: [pacman-dev] [PATCH 2/2] makepkg: Allow install/changelog decls to be arrays

This doesn't actually change the treatment of install and changelog varaibles, but it allows valid bash syntax to get by our parsing of these variables. Fixes FS#25827. Signed-off-by: Dave Reisner --- This is getting pretty ugly... I'm still 50/50 on whether or not we should do this. I will point out that if we choose not to merge this, there's a dozen or so packages in extra/community that need to be fixed: $ grep -lE '^(install|changelog)=$' /var/abs/*/*/PKGBUILD And just because I'm getting a little paranoid about making changes this close to release, here's the test rig I used to proof this patch: http://paste.xinu.at/XcbR/ Allan, thoughts? scripts/makepkg.sh.in | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 84221d0..f2ad16d 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1537,7 +1537,7 @@ check_sanity() { local file while read -r file; do # evaluate any bash variables used - eval file=\"$(sed 's/^\(['\''"]$$.*$\1$/\2/' <<< "$file")\" + eval file=\"$(sed 's/^($.*$)$/\1/;s/^$['\''"]$$.*$\1$/\2/' <<< "$file")\" if [[ $file && ! -f $file ]]; then error "$(gettext "%s file (%s) does not exist.")" "$i" "$file" ret=1 -- 1.7.7

Allan McRae

3:53 a.m.

New subject: [pacman-dev] [PATCH 2/2] makepkg: Allow install/changelog decls to be arrays

On 10/10/11 13:35, Dave Reisner wrote:

...

This doesn't actually change the treatment of install and changelog varaibles, but it allows valid bash syntax to get by our parsing of these variables.

Fixes FS#25827.

Signed-off-by: Dave Reisner --- This is getting pretty ugly... I'm still 50/50 on whether or not we should do this. I will point out that if we choose not to merge this, there's a dozen or so packages in extra/community that need to be fixed:

$ grep -lE '^(install|changelog)=\(' /var/abs/*/*/PKGBUILD

And just because I'm getting a little paranoid about making changes this close to release, here's the test rig I used to proof this patch:

http://paste.xinu.at/XcbR/

Allan, thoughts?

I will give this an ack. I lean ever so slightly to the side that if bash accepts that syntax then so should we. So given this is a fairly small fix, I see no point in not including it. I added a couple more cases to your tests (involving files with spaces...) and all seems good to me. Allan

Dave Reisner

3:57 a.m.

New subject: [pacman-dev] [PATCH 2/2] makepkg: Allow install/changelog decls to be arrays

On Mon, Oct 10, 2011 at 01:53:38PM +1000, Allan McRae wrote:

...

On 10/10/11 13:35, Dave Reisner wrote:

...
This doesn't actually change the treatment of install and changelog varaibles, but it allows valid bash syntax to get by our parsing of these variables.

Fixes FS#25827.

Signed-off-by: Dave Reisner --- This is getting pretty ugly... I'm still 50/50 on whether or not we should do this. I will point out that if we choose not to merge this, there's a dozen or so packages in extra/community that need to be fixed:

$ grep -lE '^(install|changelog)=\(' /var/abs/*/*/PKGBUILD

And just because I'm getting a little paranoid about making changes this close to release, here's the test rig I used to proof this patch:

http://paste.xinu.at/XcbR/

Allan, thoughts?

I will give this an ack. I lean ever so slightly to the side that if bash accepts that syntax then so should we. So given this is a fairly small fix, I see no point in not including it.

I added a couple more cases to your tests (involving files with spaces...) and all seems good to me.

Allan

Ok -- pushing a slightly revised version to my working branch then which uses -e 's/ex/pr/' -e 's/ex/pr/' because I'm suddenly reminded that combining expressions isn't supported with BSD sed. d

Dan McGee

3:50 a.m.

New subject: [pacman-dev] [PATCH 1/2] dload: unhook error buffer after transfer finishes

On Sun, Oct 9, 2011 at 10:35 PM, Dave Reisner wrote:

...

Similar to what we did in edd9ed6a, disconnect the relationship with our stack allocated error buffer from the curl handle. Just as an FTP connection might have some network chatter on teardown causing the progress callback to be triggered, we might also hit an error condition that causes curl to write to our (now out of scope) error buffer.

I'm unable to reproduce FS#26327, but I have a suspicion that this should fix it.

Signed-off-by: Dave Reisner --- lib/libalpm/dload.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index 33824be..e848b30 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -377,8 +377,11 @@ static int curl_download_internal(struct dload_payload *payload, /* perform transfer */ payload->curlerr = curl_easy_perform(curl);

- /* immediately unhook the progress callback */ + /* disconnect relationships from the curl handle for things that might go out + * of scope, but could still be touched on connection teardown. This really + * only applies to FTP transfers. See FS#26327 for an example. */ curl_easy_setopt(curl, CURLOPT_NOPROGRESS, 1L); + curl_easy_setopt(curl, CURLOPT_ERRORBUFFER, (char *)0); 0, aka NULL? Don't mix integers and pointers please, and the cast thus becomes unnecessary.

...

/* was it a success? */ switch(payload->curlerr) { -- 1.7.7

Dave Reisner

4:01 a.m.

New subject: [pacman-dev] [PATCH 1/2] dload: unhook error buffer after transfer finishes

On Sun, Oct 09, 2011 at 10:50:08PM -0500, Dan McGee wrote:

...

On Sun, Oct 9, 2011 at 10:35 PM, Dave Reisner wrote:

...
Similar to what we did in edd9ed6a, disconnect the relationship with our stack allocated error buffer from the curl handle. Just as an FTP connection might have some network chatter on teardown causing the progress callback to be triggered, we might also hit an error condition that causes curl to write to our (now out of scope) error buffer.

I'm unable to reproduce FS#26327, but I have a suspicion that this should fix it.

Signed-off-by: Dave Reisner --- lib/libalpm/dload.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index 33824be..e848b30 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -377,8 +377,11 @@ static int curl_download_internal(struct dload_payload *payload, /* perform transfer */ payload->curlerr = curl_easy_perform(curl);

- /* immediately unhook the progress callback */ + /* disconnect relationships from the curl handle for things that might go out + * of scope, but could still be touched on connection teardown. This really + * only applies to FTP transfers. See FS#26327 for an example. */ curl_easy_setopt(curl, CURLOPT_NOPROGRESS, 1L); + curl_easy_setopt(curl, CURLOPT_ERRORBUFFER, (char *)0); 0, aka NULL? Don't mix integers and pointers please, and the cast thus becomes unnecessary.

Sorry, should have mentioned that this won't compile using NULL because of the typechecking curl does: dload.c:384:2: error: call to '_curl_easy_setopt_err_error_buffer' declared with attribute warning: curl_easy_setopt expects a char buffer of CURL_ERROR_SIZE as argument for this option [-Werror] d

...

...
/* was it a success? */ switch(payload->curlerr) { -- 1.7.7

4590

Age (days ago)

4590

Last active (days ago)

List overview

Download

5 comments

3 participants

participants (3)

Allan McRae
Dan McGee
Dave Reisner