Hi Yegor,
On Mon, Aug 03, 2020 at 07:07:00AM +0200, yegorslists@xxxxxxxxxxxxxx wrote:
> From: Yegor Yefremov <yegorslists@xxxxxxxxxxxxxx>
>
> Use busybox implementation as a reference.
Would you add the busybox version to the commit message for reference
and to make future updates easier?
> +/* Concatenate path and filename to new allocated buffer.
> + * Add '/' only as needed (no duplicate // are produced).
> + * If path is NULL, it is assumed to be "/".
> + * filename should not be NULL.
> + */
> +static char* concat_path_file(const char *path, const char *filename)
> +{
> + char *lc;
> +
> + if (!path)
> + path = "";
> + lc = last_char_is(path, '/');
> + while (*filename == '/')
> + filename++;
> + return xasprintf("%s%s%s", path, (lc==NULL ? "/" : ""), filename);
> +}
We already have this function in lib/libbb.c, better use it than adding
it again. You add more functions that seem to be generic busybox
functions, please check if they are better added to lib/libbb.c in case
we need them for other code.
> +static int do_untar(int argc, char *argv[])
> +{
> + archive_handle_t *handle;
> + int ret;
> +
> + if (argc < 2)
> + return COMMAND_ERROR_USAGE;
> +
> + handle = init_handle();
This allocates memory which you never free, neither in the successful
case nor in the error path.
> + handle->src_fd = open(argv[1], O_RDONLY);
> + if (handle->src_fd < 0) {
> + perror("open");
> + return 1;
> + }
> +
> + if (argc == 3) {
> + chdir(argv[2]);
Doesn't this change the current directory also in the calling shell?
Also this call can fail when the path doesn't exist or is not a
directory. You should check this.
> + }
> +
> + while(!get_header(handle))
> + ret = 0;
> +
> + if (ret)
> + printf("failed to decompress\n");
'ret' is either used uninitialized or initialized to zero.
> +
> + close(handle->src_fd);
> + return ret;
> +}
> +
> +BAREBOX_CMD_START(untar)
> + .cmd = do_untar,
> + BAREBOX_CMD_DESC("unpack a tar file")
> + BAREBOX_CMD_OPTS("INFILE [DIRECTORY]")
I don't like positional arguments very much. It's fine for the infile,
but IMO the directory should be given with an option instead.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
_______________________________________________
barebox mailing list
barebox@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
