| Message ID | 20210323065414.493386-2-dominique.martinet@atmark-techno.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/3] utils: move get_output_size from ubivol handler | expand |
Hi, anything I could do to help with this? For what it's worth I've set up a freebsd VM to test swupdate for the third patch of the series and... There are at least half a dozen of linux-specific headers so I didn't get as far as compiling everything.. but at least utils.c builds so I didn't break too much. Christian, I wish you good luck for when you get to it :(
On 01.04.21 07:30, Dominique Martinet wrote: > Hi, > > anything I could do to help with this? > I apply them on my test branch and I will let coverity run. > For what it's worth I've set up a freebsd VM to test swupdate for the > third patch of the series and... There are at least half a dozen of > linux-specific headers so I didn't get as far as compiling everything.. > but at least utils.c builds so I didn't break too much. > > Christian, I wish you good luck for when you get to it :( > mmhhh..if we fix this, we need then an automatic way to build it and to recognize if something get broken. I built SWUpdate for FreeBSD just for test and a long time ago, there is nothing in current CI to check if build is broken. Regards, Stefano
Stefano Babic wrote on Thu, Apr 01, 2021 at 10:55:29AM +0200: > I apply them on my test branch and I will let coverity run. Thanks! > > For what it's worth I've set up a freebsd VM to test swupdate for the > > third patch of the series and... There are at least half a dozen of > > linux-specific headers so I didn't get as far as compiling everything.. > > but at least utils.c builds so I didn't break too much. > > > > Christian, I wish you good luck for when you get to it :( > > mmhhh..if we fix this, we need then an automatic way to build it and to > recognize if something get broken. I built SWUpdate for FreeBSD just for > test and a long time ago, there is nothing in current CI to check if build > is broken. What is used for CI? the .travis.yml would make me say travis? (there's been a lot of projects who stopped since they changed their policy on open source...) If so they apparently have "experimental freebsd support" if we just request os: freebsd in the yaml; I'm not too familiar with the syntax but looking at the doc it looks like it should be as simple as adding the following section: --- os: - linux - freebsd --- (well, that and adjusting the prerequirement install script to install freebsd packages, so it's a bit more work...) Anyway I agree it's difficult to keep FreeBSD afloat unless some automatic tests run on it; I won't be using swupdate on any BSD though so it's a bit of a stretch to justify spending much time on it but if you're willing to setup CI I can give a small hand.
Hi Dominique, On 01.04.21 11:50, Dominique Martinet wrote: > Stefano Babic wrote on Thu, Apr 01, 2021 at 10:55:29AM +0200: >> I apply them on my test branch and I will let coverity run. > > Thanks! Everything fine, patches applied to mainline. > >>> For what it's worth I've set up a freebsd VM to test swupdate for the >>> third patch of the series and... There are at least half a dozen of >>> linux-specific headers so I didn't get as far as compiling everything.. >>> but at least utils.c builds so I didn't break too much. >>> >>> Christian, I wish you good luck for when you get to it :( >> >> mmhhh..if we fix this, we need then an automatic way to build it and to >> recognize if something get broken. I built SWUpdate for FreeBSD just for >> test and a long time ago, there is nothing in current CI to check if build >> is broken. > > What is used for CI? the .travis.yml would make me say travis? > (there's been a lot of projects who stopped since they changed their > policy on open source...) I know, I was involved as U-Boot maintainer. Travis limits the number of builds, but this is not currently an issue with SWUpdate, it is more for larger projects. If this becomes an issue, I will move to a gitlab instance. > > If so they apparently have "experimental freebsd support" if we just > request os: freebsd in the yaml; I'm not too familiar with the syntax > but looking at the doc it looks like it should be as simple as adding > the following section: > --- > os: > - linux > - freebsd > --- > > (well, that and adjusting the prerequirement install script to install > freebsd packages, so it's a bit more work...) Right. > > > Anyway I agree it's difficult to keep FreeBSD afloat unless some > automatic tests run on it; Exactly - it worked sometimes ago, but as I have no projects based on FreeBSD and there is no automatic tests, I do not note if it gets broken. > I won't be using swupdate on any BSD though > so it's a bit of a stretch to justify spending much time on it but if > you're willing to setup CI I can give a small hand. > Best regards, Stefano
Hi, > > [...] > > If so they apparently have "experimental freebsd support" if we just > > request os: freebsd in the yaml; I'm not too familiar with the syntax > > but looking at the doc it looks like it should be as simple as adding > > the following section: > > --- > > os: > > - linux > > - freebsd > > --- > > > > (well, that and adjusting the prerequirement install script to install > > freebsd packages, so it's a bit more work...) > > Right. > > > > > > > Anyway I agree it's difficult to keep FreeBSD afloat unless some > > automatic tests run on it; > > Exactly - it worked sometimes ago, but as I have no projects based on FreeBSD > and there is no automatic tests, I do not note if it gets broken. Yes, the same for me, it's depending on casual tests and updates for the project that uses FreeBSD. That said, haven't done tests in a long time now and even then, the whole feature set of SWUpdate is certainly not tested thoroughly (by me). CI integration would definitely help here. That said, I do consider this a distinctive feature of SWUpdate to support (Free)BSDs. > > I won't be using swupdate on any BSD though > > so it's a bit of a stretch to justify spending much time on it but if > > you're willing to setup CI I can give a small hand. Count me in. CI would be the way forward here to not miss FreeBSD degradation. Kind regards, Christian
diff --git a/core/util.c b/core/util.c index 0f66f0c72929..9e7a268f703f 100644 --- a/core/util.c +++ b/core/util.c @@ -988,40 +988,46 @@ int read_lines_notify(int fd, char *buf, int buf_size, int *buf_offset, return n; } -long long get_output_size(struct img_type *img) +long long get_output_size(struct img_type *img, bool strict) { char *output_size_str = NULL; long long bytes = img->size; if (img->compressed) { output_size_str = dict_get_value(&img->properties, "decompressed-size"); + if (!output_size_str) { + if (!strict) + return bytes; + + ERROR("image is compressed but 'decompressed-size' property was not found"); + return -ENOENT; + } bytes = ustrtoull(output_size_str, 0); - if (errno) { - ERROR("decompressed-size argument: ustrtoull failed"); + if (errno || bytes <= 0) { + ERROR("decompressed-size argument %s: ustrtoull failed", + output_size_str); return -1; } - if (bytes == 0) { - ERROR("UBIFS to be decompressed, but decompressed-size not valid"); - return -1; - } TRACE("Image is compressed, decompressed size %lld bytes", bytes); } else if (img->is_encrypted) { - output_size_str = dict_get_value(&img->properties, "decrypted-size"); + if (!output_size_str) { + if (!strict) + return bytes; + ERROR("image is encrypted but 'decrypted-size' property was not found"); + return -ENOENT; + } bytes = ustrtoull(output_size_str, 0); - if (errno){ - ERROR("decrypted-size argument: ustrtoull failed"); + if (errno || bytes <= 0) { + ERROR("decrypted-size argument %s: ustrtoull failed", + output_size_str); return -1; } - if (bytes < AES_BLK_SIZE) { - ERROR("Encrypted image size (%lld) too small", bytes); - return -1; - } TRACE("Image is crypted, decrypted size %lld bytes", bytes); } diff --git a/handlers/ubivol_handler.c b/handlers/ubivol_handler.c index bf8c035e1b81..4730b8c02fbb 100644 --- a/handlers/ubivol_handler.c +++ b/handlers/ubivol_handler.c @@ -157,7 +157,7 @@ static int update_volume(libubi_t libubi, struct img_type *img, char sbuf[128]; struct ubi_vol_info *repl_vol; - bytes = get_output_size(img); + bytes = get_output_size(img, true); if (bytes <= 0) return -1; @@ -411,7 +411,7 @@ static int install_ubivol_image(struct img_type *img, int ret; if (check_ubi_autoresize(img)) { - long long bytes = get_output_size(img); + long long bytes = get_output_size(img, true); if (bytes <= 0) return -1; diff --git a/include/util.h b/include/util.h index 57e069f7e14e..adf3aac6f259 100644 --- a/include/util.h +++ b/include/util.h @@ -218,7 +218,7 @@ unsigned int count_string_array(const char **nodes); void free_string_array(char **nodes); int read_lines_notify(int fd, char *buf, int buf_size, int *buf_offset, LOGLEVEL level); -long long get_output_size(struct img_type *img); +long long get_output_size(struct img_type *img, bool strict); /* Decryption key functions */ int load_decryption_key(char *fname);
If dict_get_value() returns null, ustrtoull would previously just segfault. Since we plan on making the attribute optional for some use case, make this condition either error or return raw cpio size based on a new argument. While we are reworking the function, also make ustrtoull failures return -EINVAL and remove the obsolete bytes < AES_BLK_SIZE check that was leftover from before decrypted-size property existed. Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com> --- Changelog v1->v2: - change extra get_output_size argument from 'warn' to 'strict', and only silence the error if the attribute was missing -- there were too many conditions for it in previous version, I believe this is easier to follow. If you'd still like no extra argument at all, I would want to make the !output_size_str check silent altogether and add extra print message in ubivol handler e.g. re-add a local helper there and catch ENOENT? That's not very pretty: ------ +static long long get_output_size_error(struct img_type *img) { + long long bytes = get_output_size(img); + + /* ENOENT error is silent, print something here */ + if (bytes == -ENOENT) + ERROR("Image is compressed/encrypted but %s property was not found", + img->compressed ? "decompressed-size" : "decrypted-size"); + + return bytes; +} ------ - remove AES_BLK_SIZE check for encrypted volumes core/util.c | 34 ++++++++++++++++++++-------------- handlers/ubivol_handler.c | 4 ++-- include/util.h | 2 +- 3 files changed, 23 insertions(+), 17 deletions(-)