S3 C++: Replace the remaing c code[WIP] Fixes #11 #21

Thaodan · 2020-06-24T13:09:02Z

Currently so far:

s3_make_date(): Refactored out

llewelld

This is great, thank you. There are just a couple of style issues to fix.

There's still some lingering C code being called in S3ListResult::onFinished() that's preventing s3.h from being removed. This can be addressed separately, but I guess I wanted to check that this is intentional. Is that correct?

src/s3access.cpp

Thaodan · 2020-07-15T21:55:11Z

I don't have much knowledge of the s3 c code but this could be a replacement for the c code in C++/Qt removing the need for direct openssl use:
https://wiki.qt.io/HMAC-SHA1

llewelld · 2020-07-30T19:25:02Z

I don't have much knowledge of the s3 c code but this could be a replacement for the c code in C++/Qt removing the need for direct openssl use:
https://wiki.qt.io/HMAC-SHA1

Sorry for taking so long to reply; I hadn't noticed I'd left this question hanging. That's a nice find, but as it happens the openssl linkage will be needed for other things, so I'd prefer to stick with openssl throughout. However the usage in s3_hmac_sign() can be achieved with just a single openssl call, so it would be good to simplify it. See for example its use here.

llewelld · 2020-07-30T19:28:13Z

It should be possible to simplify the MD5 code similarly: https://www.openssl.org/docs/man1.1.1/man3/MD5.html

Thaodan · 2020-08-02T20:25:55Z

Is the md5 function used somewhere?

llewelld · 2020-08-02T20:34:20Z

Is the md5 function used somewhere?

The only instance I could see was in s3_put(), which isn't used yet (and I think probably won't be needed in the future), so feel free to drop both if you also think they're not needed.

llewelld

This is really great and looking nice and clean. I tested it, and with the "date" part added in it worked great too. I made a few style requests, but the only change that's essential to getting it to work is for the "date" line to be fixed.

llewelld · 2020-08-06T21:16:53Z

src/s3access.cpp

+        break;
+    }
+
+    QString signData = methodStr + "\n\n\n" +   + "\n/" + m_bucket + "/";


There's a date missing from here. It should be:

String signData = methodStr + "\n\n\n" + date + "\n/" + m_bucket + "/";

llewelld · 2020-08-06T21:18:51Z

src/s3access.cpp

+        methodStr = "PUT";
+        qDebug() << "PUT request";
+        break;
+    case GET:


You can remove the case GET: here to match the code further down (or add the same code at line, but best to stay consistent with it).

llewelld · 2020-08-06T21:19:17Z

src/s3access.cpp

 {
    QNetworkRequest request;
-    char *digest;
+    unsigned char digest[EVP_MAX_MD_SIZE +1 ]; // +1 for the \0 byte


The +1 is no longer needed here (since you're not adding the null byte after all).

llewelld · 2020-08-06T21:21:12Z

src/s3access.cpp

+    QString signData = methodStr + "\n\n\n" +   + "\n/" + m_bucket + "/";
+
+    if (signDataKey != nullptr)
+        signData+=signDataKey;


For consistency (and because it's a common source of error) I'd be grateful if you could include the curly brackets here.

llewelld · 2020-08-06T21:23:07Z

src/s3access.cpp

@@ -168,7 +177,6 @@ QNetworkReply *S3Access::performOp(Method method, QString const &url, QString co
        break;
    }

-    free(digest);



There's an extra line here after you've removed the free(digest). If you could remove it, that would be appreciated.

I don't see that line, I just removed that line here.

Yes, I wasn't very clear. I mean if you have this:

1. 2. statement 3.

and then remove statement, you end up with two blank lines:

1. 3.

but for purely aesthetic reasons there should only be a single blank line after the switch statement.

Moved the creation of the signData string to performOp making s3_make_date() unneeded. Added signDataKey to allow for optional signData key for methods like getFile(). Should also make adding methods that user other Methods easier. Also cleanes up the converting of date to QString and just converts it to QBytearray when needed.

Instead of using s3_hmac_sign() which does everything on its own resuse HMAC() from OpenSSL. This adds some casting that was done previously inside s3_hmac_sign() to performOp(). In addition we no longer use the deprecated HMAC_Init(HMAC_CTX *ctx, const void *key, int key_len, const EVP_MD *md) (<1.1). Removes the usage of: + s3_hmac_sign() Contributes to llewelld#11

The S3 struct is no longer used since all c-style s3 methods are no longer used. Remove all code that related to it. Removes the usage of: + s3_free() + s3_init() Contributes to llewelld#11.

llewelld · 2020-08-08T14:37:06Z

You still have [WIP] in the title, but this is good enough -- and substantial enough -- to merge in its current form. Did you have other changes in mind? If you did, it might make sense to put them in a follow-up PR, if you prefer?

Thaodan · 2020-08-08T14:40:56Z

I'd replace the s3 buckets struct if you don't mind and merge this afterwards. Then hole code of the s3 folder is no longer needed.

Thaodan mentioned this pull request Jun 24, 2020

Convert S3 client C code into C++ #11

Open

llewelld requested changes Jun 24, 2020

View reviewed changes

src/s3access.cpp Outdated Show resolved Hide resolved

src/s3access.cpp Outdated Show resolved Hide resolved

src/s3access.cpp Outdated Show resolved Hide resolved

Thaodan force-pushed the s3_client_c++ branch 4 times, most recently from 7a57eed to 472ad31 Compare June 28, 2020 23:05

Thaodan force-pushed the s3_client_c++ branch 2 times, most recently from c288979 to ff3e4e3 Compare August 2, 2020 20:18

Thaodan force-pushed the s3_client_c++ branch 5 times, most recently from ead11c8 to d8a173f Compare August 4, 2020 10:01

llewelld requested changes Aug 6, 2020

View reviewed changes

Thaodan added 4 commits August 8, 2020 07:10

Refactor out the usage of the S3 struct

b31c594

The S3 struct is no longer used since all c-style s3 methods are no longer used. Remove all code that related to it. Removes the usage of: + s3_free() + s3_init() Contributes to llewelld#11.

Add braces to if signDataKey for consistency

6de0dee

Thaodan force-pushed the s3_client_c++ branch from d8a173f to 6de0dee Compare August 8, 2020 05:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

S3 C++: Replace the remaing c code[WIP] Fixes #11 #21

S3 C++: Replace the remaing c code[WIP] Fixes #11 #21

Thaodan commented Jun 24, 2020 •

edited

Loading

llewelld left a comment

Thaodan commented Jul 15, 2020

llewelld commented Jul 30, 2020

llewelld commented Jul 30, 2020

Thaodan commented Aug 2, 2020

llewelld commented Aug 2, 2020

llewelld left a comment

llewelld Aug 6, 2020

llewelld Aug 6, 2020

llewelld Aug 6, 2020

llewelld Aug 6, 2020

llewelld Aug 6, 2020

Thaodan Aug 8, 2020

llewelld Aug 8, 2020

llewelld commented Aug 8, 2020

Thaodan commented Aug 8, 2020 via email

S3 C++: Replace the remaing c code[WIP] Fixes #11 #21

Are you sure you want to change the base?

S3 C++: Replace the remaing c code[WIP] Fixes #11 #21

Conversation

Thaodan commented Jun 24, 2020 • edited Loading

llewelld left a comment

Choose a reason for hiding this comment

Thaodan commented Jul 15, 2020

llewelld commented Jul 30, 2020

llewelld commented Jul 30, 2020

Thaodan commented Aug 2, 2020

llewelld commented Aug 2, 2020

llewelld left a comment

Choose a reason for hiding this comment

llewelld Aug 6, 2020

Choose a reason for hiding this comment

llewelld Aug 6, 2020

Choose a reason for hiding this comment

llewelld Aug 6, 2020

Choose a reason for hiding this comment

llewelld Aug 6, 2020

Choose a reason for hiding this comment

llewelld Aug 6, 2020

Choose a reason for hiding this comment

Thaodan Aug 8, 2020

Choose a reason for hiding this comment

llewelld Aug 8, 2020

Choose a reason for hiding this comment

llewelld commented Aug 8, 2020

Thaodan commented Aug 8, 2020 via email

Thaodan commented Jun 24, 2020 •

edited

Loading