Refactor package module into C #6

Open
GreekStapler wants to merge 25 commits from GreekStapler/libvieter:package into dev
First-time contributor

Finally finished test cases and other bits, I think it's ready for review.

Finally finished test cases and other bits, I think it's ready for review.
GreekStapler added 8 commits 2023-01-25 23:56:55 +01:00
After my changes to the macros, they still feel a bit hacky, but I'm content with them. I also changed the parser to assume the .PKGINFO files are always valid because they are automatically generated. The parser also assumes the same fields will always appear in the same fixed order. I made this change after checking how makepkg generated this file.
The method I was trying started to irk me when I thought of creating a test unit for it. Also fixed some other issues I found in the package_to_description function (SHA256SUM section still missing).
fix: Somehow, test file changed line endings to CRLF. Changed it back to LF.
Some checks are pending
ci/woodpecker/pr/woodpecker Pipeline is pending
efbd9eaef9
GreekStapler force-pushed package from efbd9eaef9 to 9452fea2ab 2023-01-27 23:17:31 +01:00 Compare
Jef Roosens added 1 commit 2023-01-28 09:58:47 +01:00
chore: fix merge marker in makefile
Some checks failed
ci/woodpecker/pr/lint Pipeline failed
ci/woodpecker/pr/test Pipeline failed
ci/woodpecker/pr/test-mem unknown status
b25da21fd1
GreekStapler added 1 commit 2023-01-28 10:49:30 +01:00
fix: strcat can not be used on uninitialised strings.
Some checks are pending
ci/woodpecker/pr/lint Pipeline is pending
ci/woodpecker/pr/test-mem Pipeline is pending
ci/woodpecker/pr/test Pipeline is pending
c47542fcd9
GreekStapler force-pushed package from c47542fcd9 to 72fea90e13 2023-01-28 11:20:42 +01:00 Compare
GreekStapler added 1 commit 2023-01-28 22:53:14 +01:00
refactor: Subsituted old hash generation with a proper implementation.
Some checks are pending
ci/woodpecker/pr/lint Pipeline is pending
ci/woodpecker/pr/test-mem Pipeline is pending
ci/woodpecker/pr/test Pipeline is pending
b87d6b1542
GreekStapler added 2 commits 2023-01-29 12:48:01 +01:00
refactor: Added free function for package struct.
Some checks are pending
ci/woodpecker/pr/lint Pipeline is pending
ci/woodpecker/pr/test-mem Pipeline is pending
ci/woodpecker/pr/test Pipeline is pending
de1227b97b
While going through with the renaming, I saw that I actually forgot to implement that part.
GreekStapler added 1 commit 2023-01-29 13:37:44 +01:00
chore: Also renamed structs to follow naming conventions.
Some checks failed
ci/woodpecker/pr/test Pipeline failed
ci/woodpecker/pr/test-mem unknown status
ci/woodpecker/pr/lint Pipeline failed
4cacb1534f
Jef Roosens requested changes 2023-01-29 13:42:07 +01:00
Jef Roosens left a comment
Owner

In general, the code isn't formatted at all, but that can be fixed by a simple make fmt.

The dynarray could should probably be moved to its own module instead, with it working with void * instead of just char *; might be useful somewhere else.

Thanks for the PR :)

In general, the code isn't formatted at all, but that can be fixed by a simple `make fmt`. The dynarray could should probably be moved to its own module instead, with it working with `void *` instead of just `char *`; might be useful somewhere else. Thanks for the PR :)
@ -0,0 +20,4 @@
/*
* Deallocate a package.
*/
void vieter_package_free(Pkg ** ptp);

I don't personally use this style of free function right now, but it might be interesting to migrate all free functions to this style indeed. I'm assuming the pointer-to-pointer is so you can set the variable containing the pointer to NULL as well?

I don't personally use this style of free function right now, but it might be interesting to migrate all free functions to this style indeed. I'm assuming the pointer-to-pointer is so you can set the variable containing the pointer to NULL as well?
Author
First-time contributor

Yeah, dangling pointers can cause some real headaches, so I prefer to have free functions set the pointer to NULL themselves to make it less error prone.

Yeah, dangling pointers can cause some real headaches, so I prefer to have free functions set the pointer to NULL themselves to make it less error prone.
@ -0,0 +1,158 @@
/*********************************************************************

I'm not quite sure yet where I'd like to place third-party code (in case we start adding more stuff like this), but considering this is the only place this is used, just leaving it here for now is fine.

I'm not quite sure yet where I'd like to place third-party code (in case we start adding more stuff like this), but considering this is the only place this is used, just leaving it here for now is fine.
@ -0,0 +4,4 @@
#define SMALL_BUFF_SIZE 128
#define ADD_STRING(section, field) if (info->field != 0) { \
snprintf(aux, SMALL_BUFF_SIZE, section, info->field); \

General note for snprintf usage: what happens if the value is larger than the buffer? Is it simply cut off? Cuz if so, we shouldn't be making assumptions about the length of the input.

General note for `snprintf` usage: what happens if the value is larger than the buffer? Is it simply cut off? Cuz if so, we shouldn't be making assumptions about the length of the input.
Author
First-time contributor

I used snprintf because I didn't want to risk any buffer overflows, but it can truncate perfectly valid inputs if they are long enough (e.g. very long url). I'll turn aux into a malloc'd string and use the return value of snprintf to reallocate memory as is necessary without any overflows.

I used `snprintf` because I didn't want to risk any buffer overflows, but it can truncate perfectly valid inputs if they are long enough (e.g. very long url). I'll turn `aux` into a malloc'd string and use the return value of `snprintf` to reallocate memory as is necessary without any overflows.
@ -0,0 +18,4 @@
buff_size *= 2; \
} \
strcat(description, aux); \
while (info->field->array[i] != NULL) { \

This check is not safe, the dynarray code doesn't make any guarantees about empty fields being NULL, they're simply malloc'ed and not initialised.

This check is not safe, the dynarray code doesn't make any guarantees about empty fields being NULL, they're simply malloc'ed and not initialised.
@ -0,0 +38,4 @@
static size_t ignored_words_len = sizeof(ignored_names) / sizeof(char *);
Pkg *vieter_package_init() {
return calloc(sizeof(pkg_info), 1);

Shouldn't this be sizeof(a package)?

Shouldn't this be `sizeof(a package)`?
GreekStapler marked this conversation as resolved
@ -0,0 +58,4 @@
// Exit early if we weren't able to successfully open the archive for reading
if (r != ARCHIVE_OK) {
return NULL;

Returning NULL when a function fails doesn't say anything about why the function failed. A better API imo (which I also use in the heap module) is for the function to return an enum value intead, indicating the success of the function. The output is written to a pointer-to-pointer that is passed as the first argument of the function.

See the heap module for an example.

Returning `NULL` when a function fails doesn't say anything about why the function failed. A better API imo (which I also use in the heap module) is for the function to return an enum value intead, indicating the success of the function. The output is written to a pointer-to-pointer that is passed as the first argument of the function. See the heap module for an example.
@ -0,0 +103,4 @@
// Get size of file
struct stat stats;
if (stat(pkg_path, &stats) != 0) {

Same here

Same here
@ -0,0 +121,4 @@
return pkg;
}
void vieter_package_sha256sum(Pkg *pkg, char *res) {

As mentioned on matrix, this function should become a streaming one.

As mentioned on matrix, this function should become a streaming one.
@ -0,0 +152,4 @@
// We transform the first half byte into the second character to keep
// each byte from becoming reversed in the final string
half_byte = hash[i] & 0b1111;
if (half_byte < 10) {

Oh smart, no array indexing required, didn't even think of that ;p

Oh smart, no array indexing required, didn't even think of that ;p
@ -0,0 +173,4 @@
char *vieter_package_to_description(Pkg *pkg) {
pkg_info *info = pkg->info;
size_t buff_size = 1024;

This should also use SMALL_BUFF_SIZE I think, just for consistency.

This should also use `SMALL_BUFF_SIZE` I think, just for consistency.
@ -0,0 +181,4 @@
// special case for FILENAME
// assuming .pkg.tar.zst; other formats are valid, this should account for that
snprintf(aux, SMALL_BUFF_SIZE, "%%FILENAME%%\n%s-%s-%s.pkg.tar.zst", info->name, info->version,

Vieter doesn't only supports zstd-compressed archives. Currently it also accepts xz- or gzip-compressed archives, so this should be accounted for.

This choice was made noteably because Archlinux ARM uses xz-compressed archives instead of zstd.

Vieter doesn't only supports zstd-compressed archives. Currently it also accepts xz- or gzip-compressed archives, so this should be accounted for. This choice was made noteably because Archlinux ARM uses xz-compressed archives instead of zstd.
@ -0,0 +3,4 @@
#include "vieter_package_info.h"
#define PKG_INFO_STRING(key_ptr, field) if ((value_ptr = strstr(value_ptr, key_ptr)) != NULL) { \
value_ptr += strlen(key_ptr);\

How does this work? strstr doens't write any terminating NULL bytes for the substring or anything, so won't this strlen call return the entire remaining size of the pkginfo string?

How does this work? `strstr` doens't write any terminating NULL bytes for the substring or anything, so won't this `strlen` call return the entire remaining size of the pkginfo string?
Author
First-time contributor

The strlen is called on key_ptr which is just a small self contained string (e.g. "\ngroup = ") that is null terminated.

The pkginfo string in here is value_ptr.

The `strlen` is called on `key_ptr` which is just a small self contained string (e.g. "\ngroup = ") that is null terminated. The pkginfo string in here is `value_ptr`.
GreekStapler added 4 commits 2023-01-31 22:02:21 +01:00
Jef Roosens added 2 commits 2023-02-02 11:19:52 +01:00
chore(package): ran fmt
Some checks failed
ci/woodpecker/pr/test Pipeline failed
ci/woodpecker/pr/test-mem unknown status
ci/woodpecker/pr/lint Pipeline was successful
655daff485
GreekStapler added 1 commit 2023-02-02 11:54:15 +01:00
fix: Initialise variables to keep CI from breaking.
Some checks are pending
ci/woodpecker/pr/lint Pipeline is pending
ci/woodpecker/pr/test-mem Pipeline is pending
ci/woodpecker/pr/test Pipeline is pending
c0925fec14
GreekStapler added 1 commit 2023-02-02 12:34:08 +01:00
refactor: Package extension now depends on what compression method was detected.
Some checks failed
ci/woodpecker/pr/lint Pipeline was successful
ci/woodpecker/pr/test Pipeline failed
ci/woodpecker/pr/test-mem unknown status
da7e41e8ca
GreekStapler added 1 commit 2023-02-02 13:37:14 +01:00
fix: Initialised variable to keep CI from breaking (again).
Some checks failed
ci/woodpecker/pr/lint Pipeline was successful
ci/woodpecker/pr/test Pipeline was successful
ci/woodpecker/pr/test-mem Pipeline failed
b9dbb8af5c
GreekStapler added 1 commit 2023-02-02 20:21:28 +01:00
fix: Plug memory leaks and fix strings that were not null terminated.
All checks were successful
ci/woodpecker/pr/lint Pipeline was successful
ci/woodpecker/pr/test Pipeline was successful
ci/woodpecker/pr/test-mem Pipeline was successful
e5f0ac8dec
Jef Roosens added 1 commit 2023-02-23 09:59:14 +01:00
refactor: move dynarray into own module
All checks were successful
ci/woodpecker/pr/lint Pipeline was successful
ci/woodpecker/pr/test Pipeline was successful
ci/woodpecker/pr/test-mem Pipeline was successful
caee502382
GreekStapler force-pushed package from caee502382 to 1f9bd26ae5 2023-03-09 23:39:44 +01:00 Compare
This repository is archived. You cannot comment on pull requests.
No reviewers
No milestone
No project
No assignees
2 participants
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: vieter-v/libvieter#6
No description provided.