Add 'swupd clean' command #383
Conversation
|
This is missing Manifest*delta in statedir files. I'll add those later if the idea gets accepted. |
|
@cmarcelo we have conflicts here. |
|
Ping |
src/clean.c
Outdated
| struct stat stat; | ||
| ret = lstat(file, &stat); | ||
| if (ret != 0) { | ||
| fprintf(stderr, "couldn't remove file %s: %s\n", file, strerror(errno)); |
There was a problem hiding this comment.
Changed to "couldn't access" file.
src/clean.c
Outdated
| if (ret == 0) { | ||
| stats.files_removed++; | ||
| } | ||
| next: |
There was a problem hiding this comment.
If you moved "file" out one level of scopt, you could move these 2 lines to the start of the loop and replace "goto next" with "continue".
src/clean.c
Outdated
| dir = opendir(path); | ||
| if (!dir) { | ||
| ret = errno; | ||
| goto end; |
There was a problem hiding this comment.
"goto end" will cause a segmentation violation.
Just "return errno", and remove the "end:" label.
src/clean.c
Outdated
|
|
||
| static bool is_all_digits(const char *s) | ||
| { | ||
| while (*s) { |
There was a problem hiding this comment.
I know I am not going to win the argument, but this should be written as a for loop.
There was a problem hiding this comment.
You mean for (; *s; s++) or taking the length and using indices?
There was a problem hiding this comment.
Changed to for (; *s; s++).
src/clean.c
Outdated
| static const size_t prefix_len = sizeof(prefix) - 1; | ||
|
|
||
| const char *name = entry->d_name; | ||
| if (strlen(name) < prefix_len) { |
There was a problem hiding this comment.
I would have used strncmp rather than the 2 separate tests and to show that we are working on string data.
You only need the strlen(name) < prefix_len test to make sure you are not reading beyond the end of the name array,
There was a problem hiding this comment.
That is much better! Done.
src/clean.c
Outdated
| goto close_then_end; | ||
| } | ||
|
|
||
| char *tmp = malloc(size + 1); |
There was a problem hiding this comment.
Not sure having tmp adds anything here rather than using contents. The compiler will probably optimize tmp out anyway.
There was a problem hiding this comment.
Control flow changed a bit so tmp is gone. It was an attempt to not "fixing" contents for the return call, but had issues.
src/clean.c
Outdated
|
|
||
| ret = fread(tmp, size, 1, f); | ||
| if (ret != 1) { | ||
| free_string(&tmp); |
There was a problem hiding this comment.
should call free, not free_string. Yes we know how free_string works, and it will do the right thing, but it is breaking the abstraction. One day we might make string_or_die use alloca and make free_string just set the pointer to NULL at which time this will turn into a memory leak.
src/clean.c
Outdated
| goto close_then_end; | ||
| } | ||
|
|
||
| contents = tmp; |
There was a problem hiding this comment.
You need to add the terminating NUL to the data
src/clean.c
Outdated
| } | ||
| } | ||
|
|
||
| while (true) { |
There was a problem hiding this comment.
Kept as is. It did not look very good in practice since the test doesn't cover all the breaking conditions (we have a case where the stream is over, and ret == 0).
src/clean.c
Outdated
| } | ||
|
|
||
| end: | ||
| free_string(&mom_contents); |
There was a problem hiding this comment.
free(mom_contents) rather than free_string
Removing those now. One improvement would be always remove those regardless of the date. |
|
Thanks for the review, Icarus. |
| if (errno) { | ||
| ret = errno; | ||
| } | ||
| break; |
There was a problem hiding this comment.
Optional. Since this is the only break, you could put the closedir() and return here.
Probably not worth it.
| ret = unlink(file); | ||
| } | ||
| if (ret != 0) { | ||
| fprintf(stderr, "couldn't remove file %s: %s\n", file, strerror(errno)); |
There was a problem hiding this comment.
Possibly remove the word "file" from the error message?
This command removes old files from statedir. The interface is a bit opaque so there will be room to modify the policy later. By default it will delete staged files that are older than DAYS_TO_KEEP_FILES. For manifests this heuristic is not good as older manifests might still be used for the current OS version being run, so also ensure that those do not get deleted. This prevents 'swupd search' to redownload files. The default behavior should be suitable to use in combination with automatic updates, to control the size of state dir. There is also an --all option to remove all the state files regardless of dates and usage. To avoid "disasters" in case some paths are set wrong, the command explicitly delete patterns of files create by swupd.
This command removes old files from statedir. The interface is a bit
opaque so there will be room to modify the policy later.
By default it will delete staged files that are older than
DAYS_TO_KEEP_FILES. For manifests this heuristic is not good as older
manifests might still be used for the current OS version being run, so
also ensure that those do not get deleted. This prevents 'swupd
search' to redownload files.
The default behavior should be suitable to use in combination with
automatic updates, to control the size of state dir. There is also an
--all option to remove all the state files regardless of dates and
usage.
To avoid "disasters" in case some paths are set wrong, the command
explicitly delete patterns of files create by swupd.