Refactor package module into C #6

Open
GreekStapler wants to merge 25 commits from GreekStapler/libvieter:package into dev

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
d252c55fb0 refactor: Modified .PKGINFO parser not to rely on gotos and continues. Parser is now optimistic and assumes .PKGINFO file is valid.
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.
3d69c8edeb refactor: Decided to not return char** in function that creates the package description after all, now returns char*.
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).
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
ci/woodpecker/pr/lint Pipeline failed Details
ci/woodpecker/pr/test Pipeline failed Details
ci/woodpecker/pr/test-mem unknown status Details
b25da21fd1
chore: fix merge marker in makefile
GreekStapler added 1 commit 2023-01-28 10:49:30 +01:00
ci/woodpecker/pr/lint Pipeline is pending Details
ci/woodpecker/pr/test-mem Pipeline is pending Details
ci/woodpecker/pr/test Pipeline is pending Details
c47542fcd9
fix: strcat can not be used on uninitialised strings.
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
ci/woodpecker/pr/lint Pipeline is pending Details
ci/woodpecker/pr/test-mem Pipeline is pending Details
ci/woodpecker/pr/test Pipeline is pending Details
b87d6b1542
refactor: Subsituted old hash generation with a proper implementation.
GreekStapler added 2 commits 2023-01-29 12:48:01 +01:00
ci/woodpecker/pr/lint Pipeline is pending Details
ci/woodpecker/pr/test-mem Pipeline is pending Details
ci/woodpecker/pr/test Pipeline is pending Details
de1227b97b
refactor: Added free function for package struct.
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
ci/woodpecker/pr/test Pipeline failed Details
ci/woodpecker/pr/test-mem unknown status Details
ci/woodpecker/pr/lint Pipeline failed Details
4cacb1534f
chore: Also renamed structs to follow naming conventions.
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?

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.

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?

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
ci/woodpecker/pr/test Pipeline failed Details
ci/woodpecker/pr/test-mem unknown status Details
ci/woodpecker/pr/lint Pipeline was successful Details
655daff485
chore(package): ran fmt
GreekStapler added 1 commit 2023-02-02 11:54:15 +01:00
ci/woodpecker/pr/lint Pipeline is pending Details
ci/woodpecker/pr/test-mem Pipeline is pending Details
ci/woodpecker/pr/test Pipeline is pending Details
c0925fec14
fix: Initialise variables to keep CI from breaking.
GreekStapler added 1 commit 2023-02-02 12:34:08 +01:00
ci/woodpecker/pr/lint Pipeline was successful Details
ci/woodpecker/pr/test Pipeline failed Details
ci/woodpecker/pr/test-mem unknown status Details
da7e41e8ca
refactor: Package extension now depends on what compression method was detected.
GreekStapler added 1 commit 2023-02-02 13:37:14 +01:00
ci/woodpecker/pr/lint Pipeline was successful Details
ci/woodpecker/pr/test Pipeline was successful Details
ci/woodpecker/pr/test-mem Pipeline failed Details
b9dbb8af5c
fix: Initialised variable to keep CI from breaking (again).
GreekStapler added 1 commit 2023-02-02 20:21:28 +01:00
ci/woodpecker/pr/lint Pipeline was successful Details
ci/woodpecker/pr/test Pipeline was successful Details
ci/woodpecker/pr/test-mem Pipeline was successful Details
e5f0ac8dec
fix: Plug memory leaks and fix strings that were not null terminated.
Jef Roosens added 1 commit 2023-02-23 09:59:14 +01:00
ci/woodpecker/pr/lint Pipeline was successful Details
ci/woodpecker/pr/test Pipeline was successful Details
ci/woodpecker/pr/test-mem Pipeline was successful Details
caee502382
refactor: move dynarray into own module
GreekStapler force-pushed package from caee502382 to 1f9bd26ae5 2023-03-09 23:39:44 +01:00 Compare
Some checks are pending
ci/woodpecker/pr/lint Pipeline is pending
Required
Details
ci/woodpecker/pr/test-mem Pipeline is pending
Required
Details
ci/woodpecker/pr/test Pipeline is pending
Required
Details
This Pull Request has changes requested by an official reviewer.
You are not authorized to merge this pull request.
Sign in to join this conversation.
There is no content yet.