Date: Mon, 09 Jan 2006 17:30:27 +0000 From: Gavin Atkinson <gavin.atkinson@ury.york.ac.uk> To: Martin Cracauer <cracauer@cons.org> Cc: Dag-Erling =?ISO-8859-1?Q?Sm=F8rgrav?= <des@des.no>, freebsd-current@freebsd.org, Colin Percival <cperciva@freebsd.org> Subject: Re: fetch extension - use local filename from content-dispositionheader (new diff) Message-ID: <1136827827.66132.30.camel@buffy.york.ac.uk> In-Reply-To: <20060109113634.A17206@cons.org> References: <20051229221459.A17102@cons.org> <868xu22mmp.fsf@xps.des.no> <200512301856.28800.jhb@freebsd.org> <200512310115.40490.jhb@freebsd.org> <20051231015102.A51804@cons.org> <43B66EF1.4020906@freebsd.org> <20060109113634.A17206@cons.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 2006-01-09 at 11:36 -0500, Martin Cracauer wrote: > All right, I hope we all calmed down a little. > > To make people a little happier, I changed the meaning of the -O > flag. > > Previously giving this non-argument -O flag would use the > Content-Disposition header after a basic safey check. > > Now this flag takes an expected filename as an argument. If the > argument is given, the server-supplied name will only be used if it > matches the expected filename. If it doesn't the transport is aborted > after reading the header. > > This will be useful when ports distfiles are distributed from servers > using this mechanism, which is bound to happen (if it doesn't > already). This way a port can say "download this URL", where the URL > is some random php script as a filename, such as > http://foo.com/download?fileid=3682, and download and save it only if the > server gave the name of the expected distfile. So we would catch it > if we make a mistake in the URL or if the server changes the mapping. > Note that this will abort before actually downloading the file. > > For the rest of us who need this for attachments in Bugzilla and > forums without knowing the expected filename you can give "." which > means use the server-supplied name as is, after the previously > mentioned safety checks. I'm not sure I like the choice of ".", but can't think of a better choice. A couple of issues with the patch: + for (i = 0; s[i] != '"' && i < length - 1; i++) { + if (s[i] < ' ') { + name_altered++; + target[i] = '_'; + } else { Maybe better off using iscntrl(3) rather than "(s[i] < ' ')"? + return NULL; These should be "return (NULL);" per style(9) - if (us.size == -1) - printf("Unknown\n"); - else - printf("%jd\n", (intmax_t)us.size); + if (O_flag) { + if (us.content_disposition[0]) + printf("%s\n", us.content_disposition); + else + printf("No_filename_supplied\n"); + } else { + if (us.size == -1) + printf("Unknown_size\n"); + else + printf("%jd\n", (intmax_t)us.size); + } I don't like the underscores in this output. You're also changing the output in the case of "-s" on it's own, which could break scripts (the most likely user for the -s option). There's also some overlap with the -Q option, I'm not sure it's needed at all. Why not, in the -O case, always print the filename unless -q is specified? + /* start the transfer */ You move this block of code to before the checks for "-r" and subsequent stat(2) of the local file - I haven't tested it, but I suspect this will break the -r option's ability to resume a partially downloaded file. + exit(34); Why 34? The man page says fetch will return 1 on failure. I wonder if this should be replaced with a "goto failure;" instead? Other than those points, the patch seems good. I can see a definite need for this functionality. Gavin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1136827827.66132.30.camel>