On 2025/9/30 17:25, zhaoyifan (H) wrote:
On 2025/9/30 17:06, Gao Xiang wrote:
Hi Yifan,
On 2025/9/30 16:40, Yifan Zhao wrote:
From: zhaoyifan <[email protected]>
`mkfs.erofs` failed to generate image from Huawei OBS with the following
command:
mkfs.erofs --s3=<endpoint>,urlstyle=vhost,sig=2 s3.erofs test-bucket
because it mistakenly generated a url with repeated '/':
https://test-bucket.<endpoint>//<keyname>
In fact, the splitting of bucket name and path has already been performed prior
to the call to `s3erofs_prepare_url`, and this function does not need to handle
this logic. This patch simplifies this part accordingly and fixes the problem.
Fixes: 29728ba8f6f6 ("erofs-utils: mkfs: support EROFS meta-only image generation
from S3")
Signed-off-by: Yifan Zhao <[email protected]>
---
lib/remotes/s3.c | 35 ++++++++++-------------------------
1 file changed, 10 insertions(+), 25 deletions(-)
diff --git a/lib/remotes/s3.c b/lib/remotes/s3.c
index 2e7763e..2bd5322 100644
--- a/lib/remotes/s3.c
+++ b/lib/remotes/s3.c
@@ -41,17 +41,16 @@ struct s3erofs_curl_request {
static int s3erofs_prepare_url(struct s3erofs_curl_request *req,
const char *endpoint,
- const char *path, const char *key,
I really think we should at least add a unittest for this.
Hi Xiang,
I agree that adding unit tests is necessary, but the issue is that it's
difficult for us to verify the validity of the URL (and accompanying request
headers) unless we actually make a request to a remote S3 service. For example,
the issue described in this patch only occurs with Huawei Cloud OBS, not with
Alibaba Cloud OSS.
I suggest that as a first step, we could perform basic rule-based validation of
the URL in unit tests—such as checking whether the canonical query matches the
URI, etc. And I will do it before submitting any patches modifying the url
perparation logic in s3erofs implementaion.
I think we could just add some simple basic patterns first,
which we are all in agree that it should be generated as-is
to avoid stupid regression due to code changes.
Otherwise if there is lack of some unit tests, this piece
could be broken later.
Thanks,
Gao Xiang
In the future, we could integrate tools that simulate an S3 service (e.g.,
https://github.com/adobe/S3Mock) as part of our CI testing pipeline (although
this still isn't sufficient to cover the compatibility differences across
various cloud providers' S3 interfaces.... )
What's your opinion?
Thanks,
Yifan
you could simply add
#ifdef TEST
int main(int argc, char argv[])
{
testfunc1(); // and use assert() if test fails
testfunc2();
}
#endif
and use gcc -o s3_test -Iinclude -lcurl lib/remote/s3.c to generate a test
program.
Thanks,
Gao Xiang
