On 10/03/2002 at 10:00 AM, Christoph Hellwig wrote:
> > +#ifdef __KERNEL__
> Nuke this. kernel he3aders aren't for userspace anyway.
At one point, userspace did share this header file.
That hasn't been the case for a while, so this has
been removed.
> > +#define EVMS_CHECK_MEDIA_CHANGE _IO(EVMS_MAJOR, >
EVMS_CHECK_MEDIA_CHANGE_NUMBER)
> > +
> > +#define EVMS_REVALIDATE_DISK_STRING "EVMS_REVALIDATE_DISK"
> > +#define EVMS_REVALIDATE_DISK _IO(EVMS_MAJOR,
EVMS_REVALIDATE_DISK_NUMBER)
> Can't you use normal revalidate/media change operations?
For most plugins, except the device manager, this operation
was nothing more than an exercise in routing the operation
requests to the underlying devices. Since this routing code
already exists in the ioctl interface, it seemed reasonable
to just reuse it for this purpose as well. As a result,
each plugin had to add 0 lines of code to support this.
> > +
> > +#define EVMS_OPEN_VOLUME_STRING "EVMS_OPEN_VOLUME"
> > +#define EVMS_OPEN_VOLUME _IO(EVMS_MAJOR,
EVMS_OPEN_VOLUME_NUMBER)
> > +
> > +#define EVMS_CLOSE_VOLUME_STRING "EVMS_CLOSE_VOLUME"
> > +#define EVMS_CLOSE_VOLUME _IO(EVMS_MAJOR,
EVMS_CLOSE_VOLUME_NUMBER)
> if you need open/close ioctl you got some abstraction wrong..
In EVMS, because of removable media devices, we chose to
open/close the underlying devices only when an actual
open/close is received for a volume residing on the
devices. This allows removable media devices to remain
unlocked until they are in use.
> > +/**
> > + * struct evms_sector_io_pkt - sector io ioctl packet definition
> > + * @disk_handle: disk handle of target device
> > + * @io_flag: 0 = read, 1 = write
> > + * @starting_sector: disk relative starting sector
> > + * @sector_count: count of sectors
> > + * @buffer_address: user buffer address
> > + * @status: return operation status
> > + *
> > + * ioctl packet definition for EVMS_SECTOR_IO ioctl
> > + **/
> > +struct evms_sector_io_pkt {
> > + u64 disk_handle;
> > + s32 io_flag;
> > + u64 starting_sector;
> > + u64 sector_count;
> > + u8 *buffer_address;
> > + s32 status;
> > +};
> You don't use an ioctl to read into a user supplied buffer??
This ioctl is used by our userspace tools. EVMS stores
metadata at the very end of devices. This ioctl was
initially done to avoid the problem of "short writes"
to those last sectors. It also allows the core to
control which devices userspace can address.
> > +/**
> > + * struct evms_compute_csum_pkt - compute checksum ioctl packet
definition
> > + * @buffer_address:
> > + * @buffer_size:
> > + * @insum:
> > + * @outsum:
> > + * @status:
> > + *
> > + * ioctl packet definition for EVMS_COMPUTE_CSUM ioctl
> > + **/
> > +struct evms_compute_csum_pkt {
> > + u8 *buffer_address;
> > + s32 buffer_size;
> > + u32 insum;
> > + u32 outsum;
> > + s32 status;
> > +};
> An ioctl to compute a checksum??
MD uses checksum routines. Rather than duplicating
this code, we provided a method, for our usertools,
of using the kernel routines, especially considering
there is multiple methods based on various
characteristics.
On Mon, Oct 07, 2002 at 11:43:30AM -0500, Mark Peloquin wrote:
> > Can't you use normal revalidate/media change operations?
>
> For most plugins, except the device manager, this operation
> was nothing more than an exercise in routing the operation
> requests to the underlying devices. Since this routing code
> already exists in the ioctl interface, it seemed reasonable
> to just reuse it for this purpose as well. As a result,
> each plugin had to add 0 lines of code to support this.
I don't think that basing kernel internal interfaces on ioctl is
a smart idea. Just add another function pionter to your operations
vector for every operation you want supported on volumes.
That way the uppermost layer can do the translation from the ugly
ioctl interface (or a future nice fs-based interface) to the operations
once instead of carrying the user-defined syscalls (aka ioctls)
around through your whole architecture.
> > if you need open/close ioctl you got some abstraction wrong..
>
> In EVMS, because of removable media devices, we chose to
> open/close the underlying devices only when an actual
> open/close is received for a volume residing on the
> devices. This allows removable media devices to remain
> unlocked until they are in use.
Again, why can't you have open/close _methods_ for it instead of abusing
the ioctl interface. It would be even nicer if you could extend
the block device operations with your new volume-mangment methods
instead of adding another API for blockdevices/disks outside the higher
level kernel code.
> > You don't use an ioctl to read into a user supplied buffer??
>
> This ioctl is used by our userspace tools. EVMS stores
> metadata at the very end of devices. This ioctl was
> initially done to avoid the problem of "short writes"
> to those last sectors. It also allows the core to
> control which devices userspace can address.
Umm. I'd suggest you fix the read/write access instead of duplicating those
two core unix syscalls in an messy ioctl API.
> > An ioctl to compute a checksum??
>
> MD uses checksum routines. Rather than duplicating
> this code, we provided a method, for our usertools,
> of using the kernel routines, especially considering
> there is multiple methods based on various
> characteristics.
Based on that argumentation we should make memcpy a syscall..
On Mon, 7 Oct 2002, Christoph Hellwig wrote:
> I don't think that basing kernel internal interfaces on ioctl is
> a smart idea. Just add another function pionter to your operations
> vector for every operation you want supported on volumes.
s/every/& generic/. Other than that, seconded. BTW, one of the pending
changes is taking the last more or less generic ioctl (HDIO_GETGEO) into
a separate method...
->ioctl() is for driver-specific crud; stuff that won't be used by
any generic application. "Make a cuckoo jump out of drive and sing
'1000 bottles of beer'" is a valid ioctl. "Get drive size" isn't.
On Mon, Oct 07, 2002 at 01:50:00PM -0400, Alexander Viro wrote:
>
>
> On Mon, 7 Oct 2002, Christoph Hellwig wrote:
>
> > I don't think that basing kernel internal interfaces on ioctl is
> > a smart idea. Just add another function pionter to your operations
> > vector for every operation you want supported on volumes.
>
> s/every/& generic/. Other than that, seconded. BTW, one of the pending
> changes is taking the last more or less generic ioctl (HDIO_GETGEO) into
> a separate method...
Well, I implied generic ioctl as the EVMS "API" (aka collection of random
ioctls) is about that generic interface.