"Siavosh Benabbas" <[EMAIL PROTECTED]> 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;
> +}
...
_______________________________________________
parted-devel mailing list
[email protected]
http://lists.alioth.debian.org/mailman/listinfo/parted-devel
