[Bug 377] [PATCH]: wget: support for -T; rework alarm() -> poll() in wget.c
Denys Vlasenko
vda.linux at googlemail.com
Sun Aug 8 00:54:11 UTC 2010
Hi Bradley,
On Saturday 07 August 2010 23:48, Bradley M. Kuhn wrote:
> This is my first time patching BusyBox, so if there is something wrong
> that I've done, please let me know and I'm happy to rewrite the patch or
> submit it more correctly according to BusyBox patch submission policies.
> (BTW, I couldn't find more instructions on procedure other than:
> http://busybox.net/fix.html Did I miss something on the website?)
Your patch format is fine.
Review:
#include "libbb.h"
#include <poll.h>
libbb.h includes sys/poll.h, no need to include it again.
+ polldata.revents = 0;
+ while (!(polldata.revents & (POLLIN|POLLPRI))) {
I'd do poll first, then check polldata.revents
- no need to clear them then. Or maybe don't check them at all...
+ pollrv = poll(&polldata, 1, 1000);
+ if (pollrv == -1) bb_perror_msg_and_die("poll");
+ progress_meter(1);
(1) safe_poll is better: it handles EINTR.
(2) checking for poll error might be not needed
- subsequent read will deal with it.
(3) maybe it's better to update progress meter after the read,
not after poll? This way it'll use updated byte count
(4) you call progress_meter(1) on every poll return.
if network is ok and data is coming in rapidly,
this means "on every packet", and progress meter
is refreshing with high frequency.
IF_FEATURE_WGET_STATUSBAR(progress_meter(0));
If FEATURE_WGET_STATUSBAR is off, progress_meter() is a no-op.
No need to bracket it in F_FEATURE_WGET_STATUSBAR() thing.
#ifdef ENABLE_FEATURE_WGET_TIMEOUT
Should be #if, not #ifdef.
ENABLE_foo is always defined (to 0 or 1).
I modified the patch and will apply it to git.
The biggest change is to teach progress meter to not update too often,
but do update on last read.
Stall indicator did not work - network socket needs to be set to
nonblocking mode, or else safe_fread() will happily do second read
after a partial one and will block (I observed it in strace).
Fixed code works like this when I unplug ethernet mid-flight:
02:40:06.824539 clock_gettime(0x1 /* CLOCK_??? */, {32591, 219966760}) = 0
02:40:06.824583 poll([{fd=3, events=POLLIN|POLLPRI, revents=POLLIN}], 1, 1000) = 1
02:40:06.824634 read(3, "\20Xw\366A\202#\254\206\251\"\17 \225\3028r"..., 512) = 416
02:40:06.824709 read(3, 0xffcdb9c8, 96) = -1 EAGAIN (Resource temporarily unavailable)
^^^^^^^^^^^^^^^^^^ crucial. was blocking on this read ^^^^^^^^^^^^^^^^
02:40:06.824754 write(5, "\20Xw\366A\202#\254\206\251\"\17 \225\3028r"..., 416) = 416
02:40:06.824832 clock_gettime(0x1 /* CLOCK_??? */, {32591, 220259461}) = 0
02:40:06.824876 poll([{fd=3, events=POLLIN|POLLPRI}], 1, 1000) = 0
02:40:07.825055 clock_gettime(0x1 /* CLOCK_??? */, {32592, 220499264}) = 0
02:40:07.825123 write(2, "\r", 1) = 1
02:40:07.825177 write(2, "patch-v2.6.35-next-2", 20) = 20
02:40:07.825228 write(2, " ", 1) = 1
02:40:07.825273 write(2, " ", 1) = 1
02:40:07.825318 write(2, "24", 2) = 2
02:40:07.825362 write(2, "% ", 2) = 2
02:40:07.825407 ioctl(2, TIOCGWINSZ, {ws_row=49, ws_col=138, ws_xpixel=0, ws_ypixel=0}) = 0
02:40:07.825457 write(2, "|", 1) = 1
02:40:07.825502 write(2, "*********************", 21) = 21
02:40:07.825551 write(2, " ", 1) = 1
... (yep, bar writing code is stooooopid) ...
02:40:07.828586 write(2, " ", 1) = 1
02:40:07.828630 write(2, "|", 1) = 1
02:40:07.828676 write(2, " ", 1) = 1
02:40:07.828720 write(2, " ", 1) = 1
02:40:07.828764 write(2, "1587", 4) = 4
02:40:07.828810 write(2, "k", 1) = 1
02:40:07.828856 write(2, " ", 1) = 1
02:40:07.828903 write(2, "0", 1) = 1
02:40:07.828947 write(2, "0", 1) = 1
02:40:07.829002 write(2, ":", 1) = 1
02:40:07.829048 write(2, "0", 1) = 1
02:40:07.829093 write(2, "0", 1) = 1
02:40:07.829138 write(2, ":", 1) = 1
02:40:07.829183 write(2, "0", 1) = 1
02:40:07.829227 write(2, "9", 1) = 1
02:40:07.829271 write(2, " ETA", 4) = 4
02:40:07.829316 poll([{fd=3, events=POLLIN|POLLPRI}], 1, 1000) = 0
02:40:08.830053 clock_gettime(0x1 /* CLOCK_??? */, {32593, 225496546}) = 0
02:40:08.830120 write(2, "\r", 1) = 1
02:40:08.830173 write(2, "patch-v2.6.35-next-2", 20) = 20
02:40:08.830223 write(2, " ", 1) = 1
02:40:08.830268 write(2, " ", 1) = 1
02:40:08.830312 write(2, "24", 2) = 2
02:40:08.830356 write(2, "% ", 2) = 2
02:40:08.830400 ioctl(2, TIOCGWINSZ, {ws_row=49, ws_col=138, ws_xpixel=0, ws_ypixel=0}) = 0
02:40:08.830450 write(2, "|", 1) = 1
02:40:08.830495 write(2, "*********************", 21) = 21
Please review the attached version.
function old new delta
retrieve_file_data 364 450 +86
bb_progress_update 615 682 +67
packed_usage 27406 27422 +16
wget_main 2440 2453 +13
static.wget_longopts 145 155 +10
progress_meter 199 159 -40
------------------------------------------------------------------------------
(add/remove: 0/0 grow/shrink: 5/1 up/down: 192/-40) Total: 152 bytes
> I attest that I am the sole copyright holder on the attached patch, and
> I license it under GPLv2-or-later, but I do understand that BusyBox as a
> whole is currently GPLv2-only.
Your wish is my command.
--
vda
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 6.patch
Type: text/x-diff
Size: 11671 bytes
Desc: not available
URL: <http://lists.busybox.net/pipermail/busybox/attachments/20100808/1a57748b/attachment-0001.bin>
More information about the busybox
mailing list