From owner-freebsd-current@FreeBSD.ORG Mon Jan 9 17:30:50 2006 Return-Path: X-Original-To: freebsd-current@freebsd.org Delivered-To: freebsd-current@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 566FC16A41F; Mon, 9 Jan 2006 17:30:50 +0000 (GMT) (envelope-from gavin.atkinson@ury.york.ac.uk) Received: from mail-gw1.york.ac.uk (mail-gw1.york.ac.uk [144.32.128.246]) by mx1.FreeBSD.org (Postfix) with ESMTP id CDC7C43D60; Mon, 9 Jan 2006 17:30:41 +0000 (GMT) (envelope-from gavin.atkinson@ury.york.ac.uk) Received: from buffy.york.ac.uk (buffy-128.york.ac.uk [144.32.128.160]) by mail-gw1.york.ac.uk (8.12.10/8.12.10) with ESMTP id k09HUR6w028029; Mon, 9 Jan 2006 17:30:27 GMT Received: from buffy.york.ac.uk (localhost [127.0.0.1]) by buffy.york.ac.uk (8.13.4/8.13.4) with ESMTP id k09HURGI067651; Mon, 9 Jan 2006 17:30:27 GMT (envelope-from gavin.atkinson@ury.york.ac.uk) Received: (from ga9@localhost) by buffy.york.ac.uk (8.13.4/8.13.4/Submit) id k09HURIo067650; Mon, 9 Jan 2006 17:30:27 GMT (envelope-from gavin.atkinson@ury.york.ac.uk) X-Authentication-Warning: buffy.york.ac.uk: ga9 set sender to gavin.atkinson@ury.york.ac.uk using -f From: Gavin Atkinson To: Martin Cracauer 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> Content-Type: text/plain Content-Transfer-Encoding: 7bit Date: Mon, 09 Jan 2006 17:30:27 +0000 Message-Id: <1136827827.66132.30.camel@buffy.york.ac.uk> Mime-Version: 1.0 X-Mailer: Evolution 2.4.2.1 FreeBSD GNOME Team Port X-York-MailScanner: Found to be clean X-York-MailScanner-From: gavin.atkinson@ury.york.ac.uk Cc: Dag-Erling =?ISO-8859-1?Q?Sm=F8rgrav?= , freebsd-current@freebsd.org, Colin Percival Subject: Re: fetch extension - use local filename from content-dispositionheader (new diff) X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 09 Jan 2006 17:30:50 -0000 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