[parted-devel] porting to FreeBSD

[Jim Meyering]

"Siavosh Benabbas" <sbenabas at gmail.com> wrote:
> 1) There is still one printf in the libparted/arch/freebsd.c file that
> is informational, e.g., it is not an exception or anything. What
> should I change this to?

Why not just remove it?

> 2) I have "#ifndef __FreeBSD__"-ed out all the si_code values that are
> not present on FreeBSD. As was already said on the list changing this
> to one #ifdef per value might be desirable.
>
> I am waiting for more comments specially on my use of
> ped_exception_throw, which I guessed.

I haven't looked closely at those.
I suggest you try to exercise a few of them.

First, a global syntactic nit:
You wrote this:
> +	if(np == NULL)
There should be a space between the "if" and the open parenthesis:
> +	if (np == NULL)

> diff --exclude=.git -NruBb parted/libparted/arch/freebsd.c parted-freebsd/libparted/arch/freebsd.c
...
> +
> +static int
> +_device_stat (PedDevice* dev, struct stat * dev_stat)

Please use "const" wherever possible.  It makes your code more
robust and more readable.  For example, add one in the line above:

  _device_stat (const PedDevice* dev, struct stat * dev_stat)

> +static int
> +_device_probe_type (PedDevice* dev)
> +{
> +	struct stat		dev_stat;
> +	char *np;
> +	PedExceptionOption	ex_status;
> +
> +#if (__FreeBSD_version >= 500000) && (__FreeBSD_version < 600000)
> +	FreeBSDSpecific*	arch_specific = FREEBSD_SPECIFIC (dev);

Move this declaration "down" into the #if block where it's used.
That gets rid of these two cpp directives and makes the code
easier to follow as well.

> +#endif
> +	
> +	if (!_device_stat (dev, &dev_stat))
> +		return 0;
> +	
> +	if (!S_ISCHR(dev_stat.st_mode)) {
> +		dev->type = PED_DEVICE_FILE;
> +		return 1;
> +	}
> +
> +	/* On FreeBSD ad0 ist the ATA device */
> +	np = strrchr(dev->path, '/');
> +	if(np == NULL)
> +	{
> +		dev->type = PED_DEVICE_UNKNOWN;
> +		return 0;
> +	}
> +	np += 1; /* advance past '/' */
> +	
> +	if(strncmp(np, "ad", 2) == 0)
> +	{
> +		dev->type = PED_DEVICE_IDE;
> +
> +		/* ad0 -> channel 0, master
> +		 * ad1 -> channel 0, slave
> +		 * ad2 -> channel 1, master
> +		 * ad3 -> channel 1, slave
> +		 */

If the above comments are relevant only to versions [5.0... 6.0),
then put them inside this #if block:

> +#if (__FreeBSD_version >= 500000) && (__FreeBSD_version < 600000)
> +		switch(*(np+3))
> +		{
> +		case 0:

Shouldn't this be case '0':?
Otherwise, you're looking for the NUL byte.

> +			arch_specific->channel = 0;
> +			arch_specific->device = 0;
> +			break;
> +		case 1:
> +			arch_specific->channel = 0;
> +			arch_specific->device = 1;
> +			break;
> +		case 2:
> +			arch_specific->channel = 1;
> +			arch_specific->device = 0;
> +			break;
> +		case 4:

Shouldn't the above be "case '3':" ?
As in the byte, 3, in "/ad3" from the comment above?

> +			arch_specific->channel = 1;
> +			arch_specific->device = 1;
> +			break;

Don't you want a "default:" label here, to handle e.g., "/ad9" and "/ad"?

> +		}

Don't you want an #else here?

> +#endif
> +	}
> +	else
> +	{
> +		dev->type = PED_DEVICE_UNKNOWN;
> +	}
> +	
> +	return 1;
> +}

...
> +static PedSector
> +_device_get_length (PedDevice* dev)

This parameter, too, should be "const".
Please fix all of them.

> +{
> +	FreeBSDSpecific* arch_specific = FREEBSD_SPECIFIC (dev);
> +	off_t bytes = 0;
> +
> +	PED_ASSERT (dev->open_count > 0, return 0);
> +
> +	if(ioctl(arch_specific->fd, DIOCGMEDIASIZE, &bytes) == 0) {
> +		return bytes / dev->sector_size;
> +	}
> +	
> +	return 0;
> +}
> +
> +
> +#if (__FreeBSD_version >= 500000) && (__FreeBSD_version < 600000)
> +# define freebsd_get_ata_params freebsd_get_ata_params_compat5
> +#elif (__FreeBSD_version >= 600000)
> +# define freebsd_get_ata_params freebsd_get_ata_params_compat6
> +#else
> +# error "parted currently compiles on FreeBSD 5.X and 6.X only"
> +#endif
> +
> +#if (__FreeBSD_version >= 500000) && (__FreeBSD_version < 600000)
> +
> +static int freebsd_get_ata_params_compat5(PedDevice *dev,
> +										  struct ata_params *ap)

Very long line, above?

> +{
> +	struct ata_cmd iocmd;
> +	int fd, ret;
> +	FreeBSDSpecific*		arch_specific = FREEBSD_SPECIFIC (dev);
> +
> +	if ((fd = open("/dev/ata", O_RDWR)) == -1)

This file is being opened read/write, but below, ...

> +	{
> +		ped_exception_throw (
> +			PED_EXCEPTION_ERROR,
> +			PED_EXCEPTION_CANCEL,
> +			_("Could not open /dev/ata."));
Rather than duplicating a file name like this, declare it
  const char *dev_ata = "/dev/ata";
and use the variable in each place.

> +
> +		ret = -1;
> +		goto out;
> +	}
> +	
> +	bzero(&iocmd, sizeof(struct ata_cmd));

Don't use the deprecated bzero function.
Use memset instead.

> +	iocmd.channel = arch_specific->channel;
> +	iocmd.device = arch_specific->device;
> +	iocmd.cmd = ATAGPARM;
> +	
> +	if (ioctl(fd, IOCATA, &iocmd) == -1)
> +	{
> +		ped_exception_throw (
> +			PED_EXCEPTION_ERROR,
> +			PED_EXCEPTION_CANCEL,
> +			_("Could not get ATA parameters. Please"
> +			  "contact the maintainer."));
> +
> +		ret = -1;
> +		goto out;
> +	}
> +	
> +	if (iocmd.u.param.type[arch_specific->device]) {
> +		*ap = iocmd.u.param.params[arch_specific->device];
> +	} else
> +	{
> +		ped_exception_throw (
> +			PED_EXCEPTION_ERROR,
> +			PED_EXCEPTION_CANCEL,
> +			_("Could not get ATA parameters. Please"
> +			  "contact the maintainer."));
> +		ret = -1;
> +		goto out;
> +	}
> +
> +	ret = 0;
> +	
> +out:
> +	close(fd);

You must check for close failure.
There are two more, below.

> +	
> +	return ret;
> +}
> +
> +#elif __FreeBSD_version >= 600000
> +
> +static int freebsd_get_ata_params_compat6(PedDevice *dev,
> +										  struct ata_params *ap)

Very long line again.

> +{
> +	FreeBSDSpecific*		arch_specific = FREEBSD_SPECIFIC (dev);

Why so much space before the variable name?

> +
> +	return ioctl (arch_specific->fd, IOCATAGPARM, ap);
> +}
> +
> +#endif
> +
> +static int
> +_device_probe_geometry (PedDevice* dev)
> +{
> +	FreeBSDSpecific*		arch_specific = FREEBSD_SPECIFIC (dev);

here too.

...
> +	if (freebsd_get_ata_params (dev, &ap) == -1) {
> +		ex_status = ped_exception_throw (
> +			PED_EXCEPTION_WARNING,
> +			PED_EXCEPTION_IGNORE_CANCEL,
> +			_("Could not get identity of device %s - %s"),
> +			dev->path, strerror (errno));
> +		switch (ex_status) {
> +		case PED_EXCEPTION_CANCEL:
> +			goto error_close_dev;
> +
> +		case PED_EXCEPTION_UNHANDLED:
> +			ped_exception_catch ();
> +		case PED_EXCEPTION_IGNORE:
> +			dev->model = strdup(_("IDE"));
> +		}
> +	} else {
> +		snprintf (vendor_buf, 64, "%s/%s", ap.model, ap.revision);

don't use literal 64 here.
Rather, use sizeof vendor_buf.

> +static int
> +freebsd_close (PedDevice* dev)
> +{
> +	FreeBSDSpecific*		arch_specific = FREEBSD_SPECIFIC (dev);
> +
> +	if (dev->dirty)
> +		_flush_cache (dev);
> +	close (arch_specific->fd);
> +	return 1;
> +}
...
> +	ret = sysctlbyname("kern.geom.conftxt", conftxt, &confsize, NULL, 0);
> +	if (ret) {
> +		ped_exception_throw (
> +			PED_EXCEPTION_ERROR,
> +			PED_EXCEPTION_CANCEL,
> +			_("error reading kern.geom.conftxt from the system"));//Was printf
> +		free (conftxt);
> +		return 0;
> +	}
> +	
> +	cptr = conftxt;
> +	
> +	while (sscanf (cptr, "%*d %16s %16s",
> +				   devtype, devname) != EOF) {

What if the actual devname value is longer than 16 bytes?
Doesn't this code then silently accept it?

if you *must* use sscanf, you have to verify its return value
matches the number of format specifiers.  i.e., don't compare w/EOF.
Instead, use "... == 3".  But even that makes it hard to distinguish
EOF from a corrupted input file.

With your != EOF test, you could match just one or two, and the
2nd and/or 3rd pointers would be used uninitialized.

> +		if (strcmp(devtype, "DISK") == 0)
> +		{
> +			strncpy (fullpath, _PATH_DEV, sizeof(fullpath) - 1);
> +			fullpath[sizeof(fullpath) - 1] = '\0';
> +			strncat (fullpath, devname,
> +					 sizeof(fullpath) - strlen(fullpath) - 1);
> +			printf("Probing %s ...\n", fullpath);
> +			_ped_device_probe (fullpath);
> +			
> +			if(_partition_is_mounted_by_path (fullpath)){
> +                                ped_exception_throw (
> +                                PED_EXCEPTION_WARNING,
> +                                PED_EXCEPTION_OK,
> +                                _("WARNING: %s or parts of it are mounted!.\n"),
> +                                fullpath);//Was printf
> +			}
> +		}
> +		
> +		if((cptr = strchr(cptr, '\n')) == NULL)
> +			break;
> +		else
> +			cptr++;
> +	}
> +
> +	ped_free(conftxt);
> +	
> +	return 1;
> +}
> +
> +static void
> +freebsd_probe_all ()
> +{
> +	_probe_standard_devices ();
> +}
> +
> +/* slice_num is slice number */
> +/* part_num is the partition number in this slice */
> +/* both slice_num and part_num can be -1. */
> +/* when slice_num is -1, then we're making paths for a raw disk */
> +/* e.g. ad0a, ad0c, ad0e, etc */
> +/* when part_num is -1, then we're making a path that takes */
> +/* the whole slice e.g. ad0s0, ad0s1, ad0s5. */
> +/* ad6s2e                */
> +/*     ||                */
> +/*     | \__ part_num    */
> +/*      \___ slice_num   */
> +static char*
> +_device_get_part_path (PedDevice* dev, int slice_num, int part_num)
> +{
> +	int		path_len = strlen (dev->path);
> +	int		result_len = path_len + 16;
> +	char*		result;
> +	char	part[2] = { '\0', '\0' };
> +	char*	fmt;
> +	
> +	result = (char*) ped_malloc (result_len);
> +	if (!result)
> +		return NULL;
> +
> +	/* slice_num = -1 when covering raw disk (ad0a, ad0c, etc) */
> +	if(slice_num == -1)
> +		fmt = "%s%s";
> +	else
> +		fmt = "%ss%d%s";

There is no need for a "fmt" variable.
Remove the four lines above, and substitute the literal
format strings into the snprintf calls below.

> +	/* if part_num is -1, then the part[] array is already a null string */
> +	/* if it's not -1, then transform slice_num into a character and put it */
> +	/* in part[] */
> +	if(part_num != -1)
> +	{
> +		part[0] = (char) part_num + 'a';
> +		part[1] = '\0';
> +	}
> +	
> +	if(slice_num == -1)
> +		/* append slice number (ad0, slice 2 => ad0s2) */
> +		snprintf (result, result_len, fmt, dev->path, part);
> +	else
> +		/* append slice and partition number (ad0, slice 1, part 2 => ad0s1c) */
> +		snprintf (result, result_len, fmt, dev->path, slice_num, part);
> +	
> +	return result;
> +}
...