Hey Denys,
On Tue, Oct 6, 2009 at 8:23 AM, Denys Vlasenko <[email protected]> wrote:
> On Wednesday 30 September 2009 15:03, Magnus Damm wrote:
>> Hi everyone,
>>
>> Here comes a few patches that break out the progress meter code from
>> wget and hooks it up to the tftp client. I'd like to see this feature
>> picked up for inclusion in upstream busybox. That said, I realize that
>> the patches may be far from perfect. So if you have any ideas on how
>> to improve things please let me know and i'll do my best to rewrite
>> and resubmit.
>>
>> Apply in the following order on top of
>> 2f3f09c287f43dcad50b740793c2b467f166c058:
>> 1: busybox-git-tftp-tsize-20090930.patch
>
> I do not understand what this patch tries to achieve.
> Please describe the reason for these changes.
Sorry for the unclear code. This patch adds support for using the
"tsize" option in the busybox client. Without this patch only the tftp
server case includes "tsize" support.
> For example:
>
> -#if ENABLE_TFTPD
> if (tsize) {
> struct stat st;
> /* add "tsize", <nul>, size, <nul> */
> strcpy(cp, "tsize");
> cp += sizeof("tsize");
> + st.st_size = 0;
> fstat(local_fd, &st);
> cp += snprintf(cp, 10, "%u", (int) st.st_size) + 1;
> + tsize = 0;
> }
> -#endif
>
> Why do you remove "#if ENABLE_TFTPD"?
Because I want "tsize" to be enabled both for tftp and tftpd.
>> 2: busybox-git-wget-break-out-progress-20090930.patch
>
> +void bb_progress_init(bb_progress_t *p);
> +void bb_progress_update(bb_progress_t *p, const char *curfile,
> + off_t beg_range, off_t transferred, off_t totalsize);
> +void bb_progress_done(bb_progress_t *p);
>
> Please add FAST_FUNC to these functions. See libbb.h how it is done
> for other functions.
Sure!
> +void bb_progress_done(bb_progress_t *p)
> +{
> + (void)p;
> + fputc('\n', stderr);
> +}
>
> This function seems to be useless. Users can just bb_putchar('\n')
> themselves.
Cool, will fix.
>> 3: busybox-git-tftp-progress-20090930.patch
>
> +/* SIGALRM logic nicked from the wget applet */
> +static void progress_meter(int flag)
>
> You duplicate the code this way.
> It makes more sense to move SIGALRM code to libbb/progress.c
> in previous patch, and just call it from wget.
I'd like to share as much code as possible but the signal handling
code is accessing global variables specific to each applet. So I
decided to only put reentrant code in libbb. I believe that letting
libbb access globals may lead to very messy code. Or do you have any
good suggestions how to break it out in a cleaner way?
>> 4: busybox-git-progress-unknown-size-20090930.patch
>
> Looks ok.
Cool. I thought patch number 4 would be the most difficult change
since it modified the wget output. =)
Let me know what you think and I'll repost. Thanks for your feedback!
Cheers,
/ magnus
_______________________________________________
busybox mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox
