Date: Wed, 10 Feb 2016 13:29:47 -0800 From: Maxim Sobolev <sobomax@FreeBSD.org> To: Thomas Quinot <thomas@freebsd.org> Cc: kib@freebsd.org, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn: head/bin/dd Message-ID: <CAH7qZfuS4uwny1rixh5YvKzQWFPBV2B%2BVe_qKiXFr2%2B-iQhYSw@mail.gmail.com> In-Reply-To: <20160210203823.GA74615@melamine.cuivre.fr.eu.org> References: <201405071933.s47JXUx0046697@svn.freebsd.org> <CAH7qZfs=1vebh1jJVF2Gz1VViAaMNTip0p_oKq7Sb4yAHefwJw@mail.gmail.com> <20160210203823.GA74615@melamine.cuivre.fr.eu.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <thomas@freebsd.org> 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. > >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAH7qZfuS4uwny1rixh5YvKzQWFPBV2B%2BVe_qKiXFr2%2B-iQhYSw>