Refactor package module into C #6
No reviewers
Labels
No Label
Idea
Roadmap
bug
duplicate
enhancement
help wanted
invalid
question
wontfix
No Milestone
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: vieter-v/libvieter#6
Loading…
Reference in New Issue
There is no content yet.
Delete Branch "GreekStapler/libvieter:package"
Deleting a branch is permanent. Although the deleted branch may exist for a short time before cleaning up, in most cases it CANNOT be undone. Continue?
Finally finished test cases and other bits, I think it's ready for review.
efbd9eaef9
to9452fea2ab
c47542fcd9
to72fea90e13
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 justchar *
; 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?
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.
@ -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.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 turnaux
into a malloc'd string and use the return value ofsnprintf
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.
@ -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)
?@ -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.
@ -0,0 +103,4 @@
// Get size of file
struct stat stats;
if (stat(pkg_path, &stats) != 0) {
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.
@ -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
@ -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.@ -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.
@ -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 thisstrlen
call return the entire remaining size of the pkginfo string?The
strlen
is called onkey_ptr
which is just a small self contained string (e.g. "\ngroup = ") that is null terminated.The pkginfo string in here is
value_ptr
.caee502382
to1f9bd26ae5