Skip to content

Conversation

@PMExtra
Copy link
Contributor

@PMExtra PMExtra commented Dec 23, 2022

https://curl.se/docs/manpage.html#--fail-with-body

For now,

curl --silent https://httpstat.us/404 return 0

wget -q --content-on-error -O - https://httpstat.us/404 return 8

To append --fail-with-body argument to curl to make their behaviors becoming same.

#from wget 1.14: do not skip body on 404 error
if _contains "$(wget --help 2>&1)" "--content-on-error"; then
_ACME_WGET="$_ACME_WGET --content-on-error "
fi
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move these codes from outer into the wget initialization blocks.
So it doesn't need to duplicate evaluate $_ACME_WGET, and the --help does not need the other arguments.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个改动其实没有必要. 如果 已经初始化过了,__HTTP_INITIALIZED=1, 根本就不会再次走进来.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个改动确实没有必要性,只是为了改善可读性,使代码结构更清晰一些。用伪代码表示一下改动前的代码是:

if 使用curl
  初始化curl
  if curl 支持某个特性
    初始化特性
  fi
fi
if 使用wget
  初始化wget
fi
if 使用wget 且 支持某个特性
  初始化特性
fi

这个代码结构看着很混乱,curl的所有初始化被放在了if 使用curl这个if块里,但是wget的分成了两块。
所以我希望把它改到if 使用wget里面去,改善可读性。

@PMExtra PMExtra marked this pull request as ready for review December 23, 2022 10:50
@Neilpang
Copy link
Member

这么改的好处是啥

@PMExtra
Copy link
Contributor Author

PMExtra commented Jan 24, 2023

@Neilpang
两处改动,先说主要的:
目前acme.sh中,可以使用ACME_USE_WGET变量控制使用wget还是curl发起请求,处理响应的行为是不一致的:
wget在4xx~5xx响应码下,exit code是非0;
而curl无论响应码多少,只要服务器返回了响应,exit code都是0。

这意味着在使用curl的时候,我们不能直接通过$?来判断请求是否成功,而需要判断body。
可能对于acme协议本身的操作我们都通过body来判断请求是否成功。但是对于dns api和deploy hook的开发,现在这个情况可能就不太方便了。可能存在一些没有body或者body格式不明确的外部API,我们想要通过http status code来判断操作是否成功。目前使用acme自带的_get_post就无法确保这样的判断,因为wget实现和curl实现的行为不一致。

所以建议给curl加上--fail-with-body参数,使其行为和wget尽量一致。

@PMExtra
Copy link
Contributor Author

PMExtra commented Jan 24, 2023

@Neilpang
第二处改动是在判断curl是否支持某个特性的时候,需要使用curl --help <category>而非curl --help
因为自从近期的某个curl版本开始,curl --help不再返回完整的帮助信息,这会导致特性判断错误,明明是最新版本的curl,具备某个特性,但是因为curl --help之中不包含该关键词而导致误判。
因此,需要加上特性对应的帮助分类,来实现对新版本curl的兼容。而加上了分类的命令,在旧版本curl中也可以正确响应,旧版本curl会无视<category>参数,始终返回完整的帮助信息。

fi

if _contains "$(curl --help 2>&1)" "--globoff"; then
if _contains "$(curl --help 2>&1)" "--globoff" || _contains "$(curl --help curl 2>&1)" "--globoff"; then
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Neilpang 你看这样行了吗,先按旧版的help判断,如果找不到再按新版判断

fi

#from curl 7.76: return fail on HTTP errors but keep the body
if _contains "$(curl --help http 2>&1)" "--fail-with-body"; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里是不是也需要改成 2次判断.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里完全是一个新特性,所以没有向后兼容的需求。
而且,curl 是从7.73出现的--help <category>这个特性,而curl 7.76才出现的--fail-with-body参数。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Neilpang Neilpang merged commit c2ad1b4 into acmesh-official:dev Jan 28, 2023
@PMExtra PMExtra deleted the feature/curl_fail branch September 30, 2024 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants