svn commit: r204615 - head/sbin/newfs

[Garrett Cooper]

On Tue, Mar 2, 2010 at 8:59 PM, Xin LI <delphij at gmail.com> wrote:
> On Tue, Mar 2, 2010 at 7:19 PM, Maxim Sobolev <sobomax at freebsd.org> wrote:
>> Xin LI wrote:
>>>
>>> On Tue, Mar 2, 2010 at 6:05 PM, Maxim Sobolev <sobomax at freebsd.org> wrote:
>>>>
>>>> Author: sobomax
>>>> Date: Wed Mar  3 02:05:09 2010
>>>> New Revision: 204615
>>>> URL: http://svn.freebsd.org/changeset/base/204615
>>>>
>>>> Log:
>>>>  Teach newfs(8) to understand size modifiers for all options taking
>>>>  size or size-like argument. I.e. "-s 32k" instead of "-s 32768".
>>>>  Size parsing function has been shamelessly stolen from the truncate(1).
>>>>  I'm sure many sysadmins out there will appreciate this small
>>>>  improvement.
>>>
>>> Bikeshed: why not expand_number()?
>>
>> I did not know that function existed, but even if I did, I am really not
>> sure if adding dependency on external library just to save 200 bytes of code
>> worth it. Considering that newfs(8) is often embedded into various
>> space-tight/custom things, adding dependency could cause more harm than
>> good. In any case, I do not feel strongly about that, so I can change it to
>> use libutil if people feel like it.
>
> [Moved from svn-all@ to -hackers@]
>
> I'd prefer depending on libutil since it's installed as a /lib library
> which is usually available, as libutil is not something easily
> avoidable.
>
> By the way I'm curious why these (humanize and friends) are not
> available as libc function?  Because they are not part of POSIX
> perhaps?

Maxim,

Xin Li has a point. I ran some tests and the ad hoc parsing function
eats up more memory than expand_number(3) [*]:

$ uname -a
FreeBSD garrcoop-fbsd.cisco.com 8.0-STABLE FreeBSD 8.0-STABLE #2: Wed
Feb  3 16:57:07 PST 2010
garrcoop at garrcoop-fbsd.cisco.com:/usr/obj/usr/src/sys/LAPPY_X86  i386

$ gcc -o test_expand_number test_expand_number.c -lutil; strip
test_expand_number; stat -f '%z %b' test_expand_number
3240 8

$ gcc -o test_non-expand_number test_non-expand_number.c; strip
test_non-expand_number; stat -f '%z %b' test_non-expand_number
3756 8

This of course is dynamically linked, not statically linked; the
statically linked size is of course ridiculous:

$ gcc -static -o test_expand_number test_expand_number.c -lutil; strip
test_expand_number; stat -f '%z %b' test_expand_number
184744 364

But just to be fair the non-expand number version is bloody near the
same (only 540 bytes smaller on disk)...

$ gcc -static -o test_non-expand_number test_non-expand_number.c;
strip test_non-expand_number; stat -f '%z %b' test_non-expand_number
184204 360

Given that expand_number(3) is more established and tested, and
supports more prefixes / 64-bit numbers, doesn't it make more sense to
use expand_number(3) given the above evidence?

Thanks,
-Garrett

[*] Notes:
1. All of these numbers were obtained with non-optimized CFLAGS (-O0,
no -march values, etc).
2. All of these numbers were obtained after stripping the binaries as
shown above.

/* Using expand_number(3) */

#include <sys/types.h>
#include <inttypes.h>
#include <libutil.h>
#include <stdint.h>

int
main(void) {

	int64_t anum;

	/*
	 * SYNOPSIS
	     #include <libutil.h>

	     int
	     expand_number(const char *buf, int64_t *num);
	 */
	(void) expand_number("10G", &anum);

	return 0;

}

/* Using parselength. */

#include <sys/types.h>

/*
 * Return the numeric value of a string given in the form [+-][0-9]+[GMKT]
 * or -1 on format error or overflow.
 */
static int
parselength(const char *ls, int *sz)
{
	off_t length, oflow;
	int lsign;

	length = 0;
	lsign = 1;

	switch (*ls) {
	case '-':
		lsign = -1;
	case '+':
		ls++;
	}

#define ASSIGN_CHK_OFLOW(x, y)  if (x < y) return -1; y = x
	/*
	 * Calculate the value of the decimal digit string, failing
	 * on overflow.
	 */
	while (isdigit(*ls)) {
		oflow = length * 10 + *ls++ - '0';
		ASSIGN_CHK_OFLOW(oflow, length);
	}

	switch (*ls) {
	case 'T':
	case 't':
		oflow = length * 1024;
		ASSIGN_CHK_OFLOW(oflow, length);
	case 'G':
	case 'g':
		oflow = length * 1024;
		ASSIGN_CHK_OFLOW(oflow, length);
	case 'M':
	case 'm':
		oflow = length * 1024;
		ASSIGN_CHK_OFLOW(oflow, length);
	case 'K':
	case 'k':
		if (ls[1] != '\0')
			return -1;
		oflow = length * 1024;
		ASSIGN_CHK_OFLOW(oflow, length);
	case '\0':
		break;
	default:
		return -1;
	}

	*sz = length * lsign;
	return 0;
}

int
main(void) {

	int anum;

	(void) parselength("10G", &anum);

	return 0;

}