From owner-svn-src-all@freebsd.org Wed Feb 10 21:29:49 2016 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 63327AA47BF for ; Wed, 10 Feb 2016 21:29:49 +0000 (UTC) (envelope-from sobomax@sippysoft.com) Received: from mail-wm0-x22c.google.com (mail-wm0-x22c.google.com [IPv6:2a00:1450:400c:c09::22c]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id EE2F8366 for ; Wed, 10 Feb 2016 21:29:48 +0000 (UTC) (envelope-from sobomax@sippysoft.com) Received: by mail-wm0-x22c.google.com with SMTP id g62so44784871wme.0 for ; Wed, 10 Feb 2016 13:29:48 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sippysoft-com.20150623.gappssmtp.com; s=20150623; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; bh=RmoszIHyOiOeC9hDGBBNkjzJQMi56+Jr00tYE/9Zlws=; b=XxSpi8/YkScJbrXUrRwqhkgdUlmwE6aClc3+r/sm0jBwwfeR1OGEB+rcBqoEDuEXw7 IKIwzZ/vSMyhCAgDgGua+T44de4sKQcaeXyriOlUSMjhqGH5uA4UizrwJmMRoedQfCf8 WesPl/IFgw0HXoWsqvudWFu7a4CmE//l6rAvQWxpX6yqvyclUPTbYr9Qr5fOfpW3U6xz v633Qwf6dWHHnkHhtfsvYplwhpHvdO+Eyyx2FD/k1nf4Dd0ceJc7zLgVwiaN/x2H9GZG Ug+LQrcMiFf/KA5UrZvUHNrtm5u/JtPyGuSWjjYw4FlEtfcLhc1ZAMzA6tvwwmgbf7kS GN9A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:sender:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=RmoszIHyOiOeC9hDGBBNkjzJQMi56+Jr00tYE/9Zlws=; b=a7wtA35pAR3g77IzZwCwSlOBNSfSZZEv7/oSLjKo0TKZSKczuDYAHePcHvjK9hqbZn HDTs2iT8cZKIK4bXjLXtoK+bw1rg9opFclJ6NVbaiJvIv3+NiHWLrdyjsJkk2rKKNKyV dOl3vJSpuJo4c9flNevVlJrwsy/EPvGgyrkiW1oW6UBeQ1LU/9BS1n6KqohSg8+UDh3B O9visyg79aTNMZLWJUyUJST+h9VJw4xmWutgxADTqVSAQWsZ5KDVsCWPz1yGLbfPEl1e 5fs6Hq4RSsRdz/aq5OAe62jMdt8jUZwVs2H7kqMKIpiLILNUsN6h5LsKj/++D3J3b6fW tjdQ== X-Gm-Message-State: AG10YORSnGepYMCIsCAzhleHRpltOIUCELWbwFl84UmXZptWlZf4vhLJwQxEhf54lL6y7dOX5ucFSysB6Od1RzbA MIME-Version: 1.0 X-Received: by 10.28.225.8 with SMTP id y8mr14498095wmg.94.1455139787439; Wed, 10 Feb 2016 13:29:47 -0800 (PST) Sender: sobomax@sippysoft.com Received: by 10.27.172.16 with HTTP; Wed, 10 Feb 2016 13:29:47 -0800 (PST) In-Reply-To: <20160210203823.GA74615@melamine.cuivre.fr.eu.org> References: <201405071933.s47JXUx0046697@svn.freebsd.org> <20160210203823.GA74615@melamine.cuivre.fr.eu.org> Date: Wed, 10 Feb 2016 13:29:47 -0800 X-Google-Sender-Auth: _vfkvHRIUbv0TEz5rEFJ8P3edVs Message-ID: Subject: Re: svn: head/bin/dd From: Maxim Sobolev To: Thomas Quinot Cc: kib@freebsd.org, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.20 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 10 Feb 2016 21:29:49 -0000 The patch looks fair to me, although I have not got chance to test it yet. You probably don't need +=3D since cnt is already tested to be 0, so "=3D" would suffice. :) In any case, I've opened a bug here: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=3D207092 So please make sure to deal with it when you are done with the commit. On another note it would be nice to add some kind of basic test case for the dd to verify correct behavior while we at it. Thanks! On Wed, Feb 10, 2016 at 12:38 PM, Thomas Quinot wrote: > * Maxim Sobolev, 2016-02-10 : > > > Looking at the code in question I don't see how could it have worked. > Look > > at the following piece of code in your diff for example: > > > > + if (force && cnt =3D=3D 0) { > > + pending -=3D last_sp; > > + assert(outp =3D=3D out.= db); > > + memset(outp, 0, cnt); > > + } > > > > When the branch is taken, cnt is 0, so at the very least memset(x, y, 0= ) > is > > NOP. Later on, write(2) is conditional on cnt !=3D 0, so that it's nev= er > > taken. As a result, lseek is the last operation the file sees. > > Correct :-( > > In the context of the current logic, I think the simple fix is: > > Index: dd.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- dd.c (r=C3=A9vision 265593) > +++ dd.c (copie de travail) > @@ -470,6 +470,7 @@ > */ > if (force && cnt =3D=3D 0) { > pending -=3D last_sp; > + cnt +=3D last_sp; > assert(outp =3D=3D out.db= ); > memset(outp, 0, cnt); > } > > > Also, for what it's worth, you can use ftruncate(2) instead of write() > for > > regular sparse files to ensure correct size. That would write just as > much > > data as needed to the end. I've made a quick and dirty patch, that seem= s > to > > be working better than current code at least: > > > > http://sobomax.sippysoft.com/dd.diff > > > > Please fix. > > Leveraging ftruncate certainly seems attractive, if the destination is a > regular file. I'll look into this option. > > Thanks! > Thomas. > >