Re: Import truncate(1) from FreeBSD

[Robert Elz <kre%munnari.OZ.AU@localhost>]

    Date:        Wed, 5 Nov 2025 19:39:30 +0100
    From:        Naveen Narayanan <nneevan90%gmail.com@localhost>
    Message-ID:  <CAJ=n6_LNc9__iUwJ+voO=S_CzTTQFdJgJuu0BDsnkDD4e+VkUQ%mail.gmail.com@localhost>

A few comments - just to the list, as I don't do @gmail for e-mail.

  | +      atf_check_equal $(du -s /mnt/test_file1 | awk '{print $1}') 128
  | +      atf_check_equal $(ls -l /mnt/test_file1 | awk '{print $5}') $((10 * 1024 * 1024))

First, please respect 80 column limits (everywhere).  Just edit
in an 80 column wide window, and fix everything that wraps.

Second (and making that easier in this case), couldn't

	du -s /mnt/test_file1 | awk '{print $1}'

be just as easily (more efficiently, and using less text) be written as

	stat -f %b /mnt/test_file1

and:
	ls -l /mnt/test_file1 | awk '{print $5}'

could be

	stat -f %z /mnt/test_file1

We have tools for this kind of operation, using antique formulae from
the ancient past seems a little dated.   And yes, this would restrict
the tests to running on something with the NetBSD stat(1) - but using
ATF largely does that all by itself.

And in abs_grow_body(), alone of all uses of du as above, there is:

  | +      atf_check_equal $(du /mnt/test_file | awk '{print $1}') 192

Was there a reason for the missing -s there, or perhaps better, is there
a reason for using it in all the other cases?   This question would become
moot if the tests were switched to use stat of course.


  | +      # truncate should extend the file to the length of the reference file
  | +      atf_check -s exit:0 -o ignore -e ignore truncate -r /mnt/test_file1 /mnt/test_file2

Another 80 col issue - and no, I won't be pointing out all of them, just
in this case (the former ends up probably not needing it) indicating a
reasonable way to wrap the line:

	atf_check -s exit:0 -o ignore -e ignore \
	    truncate -r /mnt/test_file1 /mnt/test_file2

which not only avoids 80 col issues, but also makes it easier to
recognise what are the atf_check options, and what is the command
being run.

With:

  | +server="rump_server -lrumpvfs -lrumpdev_disk -lrumpfs_ffs -d key=/dev/dk,hostpath=ffs.img,size=host"

For this kind of long line, rather than wrapping it using a \ inside the ""
it is cleaner to write it something like:

	server='rump_server -lrumpvfs -lrumpdev_disk -lrumpfs_ffs'
	server="${server} -d key=/dev/dk,hostpath=ffs.img,size=host"

(and yes, the use of '' in the first line is deliberate, when there
are no expansions in the string, '' quoting is more efficient for sh,
since it doesn't need to waste time looking for potential expansions,
nor for \ escaping.)

It is not unreasonable to write it as:

	server=rump_server
	server="${server} -lrumpvfs"
	server="${server} -lrumpdev_disk"
	server="${server} -lrumpfs_ffs"
	server="${server} -d key=/dev/dk,hostpath=ffs.img,size=host"

which makes it easier if it is ever needed to remove (or add) an
option, or reorder the rump -l options (which has been known to happen).


  | +.\" Portions of this manual page were written by Ka Ho Ng <khng%FreeBSD.org@localhost>

Do we ever use that wacky user%domain@localhost nonsense in NetBSD?

I know the idea is to avoid sticking valid e-mail addresses in public
places, but don't you think that the scanners which look for them
will be wise to that trick by now?   Here one might replace FreeNSD.org
by fbsd.o  -- humans reading that will work out what it means, and
I doubt there are enough like that for the robots to bother learning it.


  | +.\" $FreeBSD$

That's not qoing to work, better to include the expanded entry from
the FreeBSD version actually used.


  | +.Nd truncate, extend the length of files, or perform space management in files

Too long.  "Adjust file sizes" perhaps?  Or "File size/space management"?


  | +.It Fl l Ar length
  | +The length of the operation range in bytes.
  | +This option must always be specified if option
  | +.Fl d
  | +is specified,

Then it isn't an option in this case, is it?
This flag must ... ??

Further, -l seems to be used only when -d is used, it isn't
as if it is optional in other cases, and just mandatory in this
one.   We probably can't do this, as it would make this be
incompatible with the FreeBSD version, but were I inventing this,
I'd just make the -d option take a size parameter ("d:" in the
getopt() string).  That should be sorted, incidentally, as should
the order of the cases in the immediately following switch, unless
there is a good reason not to (such as 2 options using the same
code).

[Aside: to me it would be useful to have an option to use with
-d to specify "only if the data in the referenced section is all
zeroes", and perhaps an alternative to -l, making -l be an option
again, for "set the size to the largest number of contiguous '\0's
starting at the offset.   Of course code would need to be written
to implement either (or both) of those.]


[...]
  | +.Ar length
  | +arguments may be suffixed with one of
  | +.Cm K ,
  | +.Cm M ,
  | +.Cm G
  | +or
  | +.Cm T
  | +(either upper or lower case) to indicate a multiple of
  | +Kilobytes, Megabytes, Gigabytes or Terabytes
  | +respectively.

Please make it clear whether this means MiB or MB (etc).
(and I expect it is the former, but, and again technically, "Kilo"
"Mega", ..., means the latter).


  | +If a file is made smaller, its extra data is lost.

technically that should be "are lost"


  | +Same as above but create the file if it does not exist:

I doubt anyone needs to be shown that deleting the -c option
will cause its effect to not apply, really, do they?

  | +.Bd -literal -offset indent
  | +truncate -r /boot/kernel/kernel test_file test_file2
  | +ls -l /boot/kernel/kernel test_file*

This probably should use NetBSD type examples, we don't use
/boot/kernel/kernel and implying that might exist could confuse
some readers.


  | +.Sh SEE ALSO
  | +.Xr fspacectl 2 ,

I assume that is some FreeBSD syscall ?
(There is no .Xr for discard 2, I'm not sure it is really
needed, but the code does use it.)


  | +.Sh STANDARDS
  | +The
  | +.Nm
  | +utility conforms to no known standards.

Just omit the section in that case.


  | diff -r 03d10f218fce -r 18356b65cc89 usr.bin/truncate/truncate.c
  | --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
  | +++ b/usr.bin/truncate/truncate.c	Tue Oct 21 22:07:50 2025 +0200
  | @@ -0,0 +1,222 @@
  | +/* $NetBSD$ */
  | +/*
  | + * SPDX-License-Identifier: BSD-2-Clause-FreeBSD
  | + *
  | + * Copyright (c) 2000 Sheldon Hearn <sheldonh%FreeBSD.org@localhost>.
  | + * All rights reserved.
  | + *
  | + * Copyright (c) 2021 The FreeBSD Foundation

There should be something in here somewhere stating which FreeBSD
version of the code all this came from (the man page as well), that
makes tracking future updates there much simpler.

It should also have a NetBSD style __RSCID() in it as well.



  | +                      oflow = sb.st_size + rsize;
  | +                      if (oflow < (sb.st_size + rsize)) {

That is not a safe way to detect arithmetic overflow.
It needs to be something more like

			if (MAX_OFF_T - sb.st_size < rsize)

which relies upon st_size not being negative, and presumes the
existence of MAX_OFF_T which I very much doubt actually exists.


  | +                      sz = rsize;
  | +                      if (sz < 0)
  | +                              sz = -sz;
  | +                      if (sb.st_size % sz) {
  | +                              round = sb.st_size / sz;

Is there something somewhere that I missed that checks
that sz != 0 in this case ?


  | +              if (do_dealloc == 1) {
  | +                      r = fdiscard(fd, off, len);
  | +              }
  | +              if (do_truncate == 1)
  | +                      r = ftruncate(fd, tsize);

Mostly you're using the KNF "always use braces", but not everywhere.
There the difference is particularly jarring.

Personally, I truly dislike that mandate, when not required by
the language spec, I prefer to leave the choice to the author,
but that choice should generally be made consistently through (at
least) each source file, or people like me wonder why the difference.


In usage():

  | +          pnam, "[-c] -s [+|-|%|/]size[K|k|M|m|G|g|T|t] file ...",

All that [K|k|...] is just ridiculous there, and even worse:

  | +          pnam, "[-c] -d [-o offset[K|k|M|m|G|g|T|t]] -l length[K|k|M|m|G|g|T|t] file ...");

as not only is the source file line too long, the output line printed
when the usage is invoked, is, as well,

Just delete all that, and add add an extra line, perhaps:

	The size offset and length values are integers, in bytes, that may
	be scaled using a K, M, G, or T suffix, in upper or lower case.

(and since it is using dehumanize_number(3), P and E work as well,
even if not particularly useful in this case.)


The code isn't written the way I'd write it, but that's neither here
nor there -- for example, for me, there are far too many variables that
are just 0 or 1, which could more easily be bits in a single variable,
which aside from being a meaningless space saving, allow (IMO) nicer code,
instead of things like

	do_relative || do_round

it would be more like

	opts & (REL | ROUND)

and if that happens to be written often, then

#define RorR	(REL | ROUND)

and use:
	opts & RorR

(some people prefer a specific "!= 0" for those (requiring extra ()),
but I don't really see the point, int type values that are not zero
are true (and 0 is false), specified by the C language definition.)

kre