Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib: replace xbps_file_hash_check_dictionary with transaction_file lookup #359

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

lib: replace xbps_file_hash_check_dictionary with transaction_file lookup #359

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b6a5762
lib: replace xbps_file_hash_check_dictionary with transaction_file lo…
Duncaen Dec 12, 2020
62c82bc
lib/transaction_files.c: also record new files from reinstall/downgrade
Duncaen Dec 17, 2020
c4c5114
include/xbps_api_impl.h: remove attributes from prototype
Duncaen Dec 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 23 additions & 2 deletions include/xbps_api_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,26 @@
#define __arraycount(x) (sizeof(x) / sizeof(*x))
#endif

typedef enum transaction_file_type {
TYPE_LINK = 1,
TYPE_DIR,
TYPE_FILE,
TYPE_CONFFILE,
} transaction_file_type_t;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically speaking *_t is reserved for libc (https://www.gnu.org/software/libc/manual/html_node/Reserved-Names.html), but everyone uses it anyway.


struct transaction_file {
const char *pkgname;
const char *pkgver;
char *sha256;
const char *target;
uint64_t size;
transaction_file_type_t type;
Comment on lines +106 to +111
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would these first 6 be the xbps_file struct? If so, I guess transaction_file_type would be switched over to file_type.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we need two struct a xbps_transaction_file and a xbps_file which is part of the former.
The idea would be to start using the xbps_file struct in places where we currently use the xbps_dictionary references to files.plist so that we gradually switch from dynamic plist types to actual structs which should help to avoid some issues at compile time through the type system.
I think xbps_file would be a simple way to start, doing this for packages would be the end goal, there are so many places that use xbps_dictionarys for packages and there are a lot of places that can be improved on the way to switching to c structs.

unsigned int index;
bool preserve;
bool update;
bool removepkg;
};

/**
* @private
*/
Expand Down Expand Up @@ -135,14 +155,15 @@ bool HIDDEN xbps_transaction_store(struct xbps_handle *, xbps_array_t, xbps_dict
int HIDDEN xbps_transaction_init(struct xbps_handle *);
int HIDDEN xbps_transaction_files(struct xbps_handle *,
xbps_object_iterator_t);
void xbps_transaction_files_free(void);
int HIDDEN xbps_transaction_fetch(struct xbps_handle *,
xbps_object_iterator_t);
int HIDDEN xbps_transaction_pkg_deps(struct xbps_handle *, xbps_array_t, xbps_dictionary_t);

struct transaction_file *xbps_transaction_file_new(struct xbps_handle *, const char *);

Comment on lines +158 to +164
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The naming is a bit confusing, in that xbps_transaction_file_new was added, but no xbps_transaction_file_free, and xbps_transaction_files_free was added, without a corresponding _new, and doesn't seem to be called at all.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes the _new does not actually create anything, it gives you a reference to a file struct that already exists from collecting all files in the transaction, "_new" because it returns the new file as opposed to the old file.

Not sure how this should be done exactly, something like xbps_transaction_get_file with either _new or _old or a parameter to choose which one you want.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xbps_transaction_files_free deletes the whole hashtable/tree, this should be called from somewhere after the transaction is done, maybe from xbps_transaction_commit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how this should be done exactly, something like xbps_transaction_get_file with either _new or _old or a parameter to choose which one you want.

I think the parameter helps avoid duplicated logic, so I'm in favor of that.

char HIDDEN *xbps_get_remote_repo_string(const char *);
int HIDDEN xbps_repo_sync(struct xbps_handle *, const char *);
int HIDDEN xbps_file_hash_check_dictionary(struct xbps_handle *,
xbps_dictionary_t, const char *, const char *);
int HIDDEN xbps_file_exec(struct xbps_handle *, const char *, ...);
void HIDDEN xbps_set_cb_fetch(struct xbps_handle *, off_t, off_t, off_t,
const char *, bool, bool, bool);
Expand Down
18 changes: 11 additions & 7 deletions lib/package_unpack.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,7 @@ unpack_archive(struct xbps_handle *xhp,
* Unpack all files on archive now.
*/
for (;;) {
struct transaction_file *file;
ar_rv = archive_read_next_header(ar, &entry);
if (ar_rv == ARCHIVE_EOF || ar_rv == ARCHIVE_FATAL)
break;
Expand Down Expand Up @@ -342,15 +343,17 @@ unpack_archive(struct xbps_handle *xhp,
continue;
}

assert(*entry_pname == '.');
file = xbps_transaction_file_new(xhp, entry_pname+1);
assert(file);

/*
* Check if current entry is a configuration file,
* that should be kept.
*/
if (!force && (entry_type == AE_IFREG)) {
buf = strchr(entry_pname, '.') + 1;
assert(buf != NULL);
keep_conf_file = xbps_entry_is_a_conf_file(binpkg_filesd, buf);
}
keep_conf_file = !force
&& (entry_type == AE_IFREG)
&& file->type == TYPE_CONFFILE;

/*
* If file to be extracted does not match the file type of
Expand Down Expand Up @@ -387,8 +390,9 @@ unpack_archive(struct xbps_handle *xhp,
}
rv = 0;
} else {
rv = xbps_file_hash_check_dictionary(
xhp, binpkg_filesd, "files", buf);
xbps_dbg_printf(xhp, "%s: %s\n", entry_pname, file->sha256);
assert(file->sha256);
Comment on lines +393 to +394
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be nice to make it clear that a (null) there is an error? Not sure.

file->sha256 ? file->sha256 : "error..."

rv = xbps_file_sha256_check(entry_pname, file->sha256);
if (rv == -1) {
/* error */
xbps_dbg_printf(xhp,
Expand Down
53 changes: 23 additions & 30 deletions lib/transaction_files.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,28 +33,10 @@
#include "xbps_api_impl.h"
#include "uthash.h"

enum type {
TYPE_LINK = 1,
TYPE_DIR,
TYPE_FILE,
TYPE_CONFFILE,
};

struct item {
char *file;
size_t len;
struct {
const char *pkgname;
const char *pkgver;
char *sha256;
const char *target;
uint64_t size;
enum type type;
unsigned int index;
bool preserve;
bool update;
bool removepkg;
} old, new;
struct transaction_file old, new;
bool deleted;
UT_hash_handle hh;
};
Expand Down Expand Up @@ -114,7 +96,7 @@ addItem(const char *file)
}

static const char *
typestr(enum type typ)
typestr(transaction_file_type_t typ)
{
switch (typ) {
case TYPE_LINK: return "symlink";
Expand Down Expand Up @@ -420,8 +402,8 @@ collect_obsoletes(struct xbps_handle *xhp)
static int
collect_file(struct xbps_handle *xhp, const char *file, size_t size,
const char *pkgname, const char *pkgver, unsigned int idx,
const char *sha256, enum type type, bool update, bool removepkg,
bool preserve, bool removefile, const char *target)
const char *sha256, transaction_file_type_t type, bool update,
bool removepkg, bool preserve, bool removefile, const char *target)
{
struct item *item;

Expand Down Expand Up @@ -526,6 +508,8 @@ collect_file(struct xbps_handle *xhp, const char *file, size_t size,
item->new.update = update;
item->new.removepkg = removepkg;
item->new.target = target;
if (sha256)
item->new.sha256 = strdup(sha256);
Comment on lines +511 to +512
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is item->new always guaranteed to be zero initialized? Might be reasonable to add an else item->new.sha256 = NULL;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally everything is zero initialized, IMHO not doing that by default when initializing the memory should only be done if you know you overwrite it immediately.

}
if (item->old.type && item->new.type) {
/*
Expand Down Expand Up @@ -565,8 +549,7 @@ collect_files(struct xbps_handle *xhp, xbps_dictionary_t d,
for (i = 0; i < xbps_array_count(a); i++) {
filed = xbps_array_get(a, i);
xbps_dictionary_get_cstring_nocopy(filed, "file", &file);
if (removefile)
xbps_dictionary_get_cstring_nocopy(filed, "sha256", &sha256);
xbps_dictionary_get_cstring_nocopy(filed, "sha256", &sha256);
size = 0;
xbps_dictionary_get_uint64(filed, "size", &size);
rv = collect_file(xhp, file, size, pkgname, pkgver, idx, sha256,
Expand All @@ -585,8 +568,7 @@ collect_files(struct xbps_handle *xhp, xbps_dictionary_t d,
xbps_dictionary_get_cstring_nocopy(filed, "file", &file);
size = 0;
xbps_dictionary_get_uint64(filed, "size", &size);
if (removefile)
xbps_dictionary_get_cstring_nocopy(filed, "sha256", &sha256);
xbps_dictionary_get_cstring_nocopy(filed, "sha256", &sha256);
#if 0
/* XXX: how to handle conf_file size */
if (removefile && stat(file, &st) != -1 && size != (uint64_t)st.st_size)
Expand Down Expand Up @@ -746,8 +728,8 @@ pathcmp(const void *l1, const void *l2)
return (a->len < b->len) - (b->len < a->len);
}

static void
cleanup(void)
void
xbps_transaction_files_free(void)
{
struct item *item, *itmp;

Expand Down Expand Up @@ -798,7 +780,8 @@ xbps_transaction_files(struct xbps_handle *xhp, xbps_object_iterator_t iter)

update = (ttype == XBPS_TRANS_UPDATE);

if (ttype == XBPS_TRANS_INSTALL || ttype == XBPS_TRANS_UPDATE) {
if (ttype == XBPS_TRANS_INSTALL || ttype == XBPS_TRANS_UPDATE
|| ttype == XBPS_TRANS_REINSTALL) {
xbps_set_cb_state(xhp, XBPS_STATE_FILES, 0, pkgver,
"%s: collecting files...", pkgver);
rv = collect_binpkg_files(xhp, obj, idx, update);
Expand Down Expand Up @@ -858,6 +841,16 @@ xbps_transaction_files(struct xbps_handle *xhp, xbps_object_iterator_t iter)
return rv;

rv = collect_obsoletes(xhp);
cleanup();
return rv;
}

struct transaction_file HIDDEN *
xbps_transaction_file_new(struct xbps_handle *xhp UNUSED, const char *path)
{
struct item *item;

if ((item = lookupItem(path)) == NULL)
return NULL;

return &item->new;
}
69 changes: 0 additions & 69 deletions lib/util_hash.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,72 +209,3 @@ xbps_file_sha256_check(const char *file, const char *sha256)

return 0;
}

static const char *
file_hash_dictionary(xbps_dictionary_t d, const char *key, const char *file)
{
xbps_object_t obj;
xbps_object_iterator_t iter;
const char *curfile = NULL, *sha256 = NULL;

assert(xbps_object_type(d) == XBPS_TYPE_DICTIONARY);
assert(key != NULL);
assert(file != NULL);

iter = xbps_array_iter_from_dict(d, key);
if (iter == NULL) {
errno = ENOENT;
return NULL;
}
while ((obj = xbps_object_iterator_next(iter)) != NULL) {
xbps_dictionary_get_cstring_nocopy(obj,
"file", &curfile);
if (strcmp(file, curfile) == 0) {
/* file matched */
xbps_dictionary_get_cstring_nocopy(obj,
"sha256", &sha256);
break;
}
}
xbps_object_iterator_release(iter);
if (sha256 == NULL)
errno = ENOENT;

return sha256;
}

int HIDDEN
xbps_file_hash_check_dictionary(struct xbps_handle *xhp,
xbps_dictionary_t d,
const char *key,
const char *file)
{
const char *sha256d = NULL;
char *buf;
int rv;

assert(xbps_object_type(d) == XBPS_TYPE_DICTIONARY);
assert(key != NULL);
assert(file != NULL);

if ((sha256d = file_hash_dictionary(d, key, file)) == NULL) {
if (errno == ENOENT)
return 1; /* no match, file not found */

return -1; /* error */
}

if (strcmp(xhp->rootdir, "/") == 0) {
rv = xbps_file_sha256_check(file, sha256d);
} else {
buf = xbps_xasprintf("%s/%s", xhp->rootdir, file);
rv = xbps_file_sha256_check(buf, sha256d);
free(buf);
}
if (rv == 0)
return 0; /* matched */
else if (rv == ERANGE || rv == ENOENT)
return 1; /* no match */
else
return -1; /* error */
}