On Thu, Aug 04, 2005 at 10:15:29AM +0530, Saripalli, Venkata Ramanamurthy 
(STSD) wrote:
> Patch 2 of 3
> This patch adds support for IDAREGNEWDISK, IDADEREGDISK, IDAGETLOGINFO
> ioctls required
> to configure LUNs dynamically on SA4200 controller using ACU.

drivers/block/cpqarray.c:

  1131  static int ida_ioctl(struct inode *inode, struct file *filep, unsigned 
int cmd, unsigned long arg)
  1132  {
  1133          drv_info_t *drv = get_drv(inode->i_bdev->bd_disk);
  1134          ctlr_info_t *host = get_host(inode->i_bdev->bd_disk);
  1135          int error;
  1136          int diskinfo[4];

Hmm... diskinfo[3] seems to be enough.

  1137          struct hd_geometry __user *geo = (struct hd_geometry __user 
*)arg;
  1138          ida_ioctl_t __user *io = (ida_ioctl_t __user *)arg;
  1139          ida_ioctl_t *my_io;
  1140
  1141          switch(cmd) {
  1142          case HDIO_GETGEO:
  1143                  if (drv->cylinders) {
  1144                          diskinfo[0] = drv->heads;
  1145                          diskinfo[1] = drv->sectors;
  1146                          diskinfo[2] = drv->cylinders;
  1147                  } else {
  1148                          diskinfo[0] = 0xff;
  1149                          diskinfo[1] = 0x3f;
  1150                          diskinfo[2] = drv->nr_blks / (0xff*0x3f);
  1151                  }
  1152                  put_user(diskinfo[0], &geo->heads);
  1153                  put_user(diskinfo[1], &geo->sectors);
  1154                  put_user(diskinfo[2], &geo->cylinders);
  1155                  put_user(get_start_sect(inode->i_bdev), &geo->start);

Mental note: export drv->heads, drv->sectors, drv->cylinders and
inode->i_bdev->bd_part->start_sect to userspace (with possible tweaking).

  1156                  return 0;
  1157          case IDAGETDRVINFO:
  1158                  if (copy_to_user(&io->c.drv, drv, sizeof(drv_info_t)))

What does drv_info_t contain? From drivers/block/cpqarray.h:

    47  typedef struct {
    48          unsigned blk_size;
    49          unsigned nr_blks;
    50          unsigned cylinders;
    51          unsigned heads;
    52          unsigned sectors;
    53          int usage_count;
    54  } drv_info_t;

Great... Same heads, sectors, cylinders we can already export. Without magic
"if (!drv->cylinders)". With extra crap for free: "usage_count". Why should
userspace know about reference count of drive? <greppery-grep> This is
not even funny...

$ grep usage_count -w -r . | grep cpq
./drivers/block/cpqarray.c:     host->usage_count++;
./drivers/block/cpqarray.c:     host->usage_count--;
./drivers/block/cpqarray.c:     if (host->usage_count > 1) {
./drivers/block/cpqarray.c:                     " revalidation (usage=%d)\n", 
host->usage_count);
./drivers/block/cpqarray.c:     host->usage_count++;
./drivers/block/cpqarray.c:     host->usage_count--;
./drivers/block/cpqarray.h:     int usage_count;
./drivers/block/cpqarray.h:     int     usage_count;

where the type of "host" is "struct ctlr_info", NOT drv_info_t.

  1159                          return -EFAULT;
  1160                  return 0;
  1161          case IDAPASSTHRU:
  1162                  if (!capable(CAP_SYS_RAWIO))
  1163                          return -EPERM;
  1164                  my_io = kmalloc(sizeof(ida_ioctl_t), GFP_KERNEL);
  1165                  if (!my_io)
  1166                          return -ENOMEM;
  1167                  error = -EFAULT;
  1168                  if (copy_from_user(my_io, io, sizeof(*my_io)))
  1169                          goto out_passthru;
  1170                  error = ida_ctlr_ioctl(host, drv - host->drv, my_io);
  1171                  if (error)
  1172                          goto out_passthru;
  1173                  error = -EFAULT;
  1174                  if (copy_to_user(io, my_io, sizeof(*my_io)))
  1175                          goto out_passthru;
  1176                  error = 0;
  1177  out_passthru:
  1178                  kfree(my_io);
  1179                  return error;
  1180          case IDAGETCTLRSIG:
  1181                  if (!arg) return -EINVAL;
  1182                  put_user(host->ctlr_sig, (int __user *)arg);
  1183                  return 0;
  1184          case IDAREVALIDATEVOLS:
  1185                  if (iminor(inode) != 0)
  1186                          return -ENXIO;
  1187                  return revalidate_allvol(host);
  1188          case IDADRIVERVERSION:
  1189                  if (!arg) return -EINVAL;
  1190                  put_user(DRIVER_VERSION, (unsigned long __user *)arg);
  1191                  return 0;

Why should userspace know anything about module version?

  1192          case IDAGETPCIINFO:
  1193          {
  1194
  1195                  ida_pci_info_struct pciinfo;
  1196
  1197                  if (!arg) return -EINVAL;
  1198                  pciinfo.bus = host->pci_dev->bus->number;
  1199                  pciinfo.dev_fn = host->pci_dev->devfn;
  1200                  pciinfo.board_id = host->board_id;

Why should usersp.... Anyone remembers what was merged first: /proc/pci/ or
this crap?

  1201                  if(copy_to_user((void __user *) arg, &pciinfo,
  1202                          sizeof( ida_pci_info_struct)))
  1203                                  return -EFAULT;
  1204                  return(0);
  1205          }
  1206
  1207          default:
  1208                  return -EINVAL;
  1209          }
  1210
  1211  }

Jens, I ask you to drop this patch. $DEITY witness,
it was wordwrapped,
wasn't incremental,
adds function which semi-duplicates already existing one,
contains irrelevant chunks,
contains commented pieces which are not debugging printks,
adds useless casts,
uses silly (in two orthogonal directions, silly) name for newly added struct,
sometimes returns -E, sometimes -1. Where it isn't a rocket science to
guess _right_ error code, it, of course, returns -1.
and generally says [censored] to CodingStyle.

The most important part, of course, is "it adds new ioctls to where there's
enough mess already".

P. S.: Al repeated many times: people, stay the fuck away from *_ioctl()
unless. Being young and idiot, I didn't want to hear the advice of experienced
hacker. Already LARTed myself.

I did "grep -i ioctl -r .". I started to dig through it. From ~10% of the log
I got ~970 ioctls. But now, after ida_ioctl(), I'm definitely going to finish
and submit Documentation/ioctl-mess.txt. I can't estimate how much time it will
take.

But a question in advance: what information about them people want to see?
Name, type of input and output seems obvious, what else?

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Reply via email to