On Wed, Jul 22, 2009 at 04:52:40PM -0700, Sage Weil wrote:
> On Thu, 23 Jul 2009, Andi Kleen wrote:
> > Sage Weil <sage@xxxxxxxxxxxx> writes:
> >
> > > A few Ceph ioctls for getting and setting file layout (striping)
> > > parameters.
> >
> > It would be good if you posted manpages for these ioctls
> > so that the interface can be reviewed. After all
> > that's intended to be used by applications, isn't it?
>
> Is there some existing manpage I might use as a reference? e.g., an XFS
> ioctl manpage or something similar?
For documenting an ioctl? The tcp(7) manpages perhaps
> I'm also happy to replace these ioctls with a virtual xattr interface
> along the lines of what Andreas proposed. That would make it much easier
> to maintain compatibility if the support layout parameters changes
> going forward.
I personally don't have a problem with them being ioctls, assuming
the interface is relatively sane. I haven't reviewed that
in detail, mostly because it's in a different patch (but it shouldn't be)
That's where the man pages would come in.
There used to be a strong "all ioctls are evil" ideology camp a few years back,
but I think those people definitely lost a lot of influence recently
and we're back to a pragmatic "where it makes sense" position.
>
> > There don't seem to be compat ioctl handlers?
>
> Oops,
Well the question is if it works, e.g. if the layout
is the same 32bit and 64bit. From a quick look the
structure seems to only contain __s32 so it might be ok.
I'm not sure what __attribute__((packed)) will do over all
the obscure architectures though.
What I find more suspicious is that it's the direct network
data structure. Presumably that's in a defined endian?
So that means the application would already need to
change to network endian order at the ioctl level?
That seems wrong if true. I think defining
a ioctl directly based on a network protocol header is
likely a bad idea.
>
> diff --git a/src/kernel/file.c b/src/kernel/file.c
> index fbf02c3..fbb5f94 100644
> --- a/src/kernel/file.c
> +++ b/src/kernel/file.c
> @@ -810,5 +810,8 @@ const struct file_operations ceph_file_fops = {
> .splice_read = generic_file_splice_read,
> .splice_write = generic_file_splice_write,
> .unlocked_ioctl = ceph_ioctl,
> +#ifdef CONFIG_COMPAT
> + .compat_ioctl = ceph_ioctl,
> +#endif
You don't need the ifdef.
> > How should the application use that if the include file is in fs/ceph?
> > Should be in include/linux I guess
> >
> > Also this file should define all the types needed for the interface,
> > especialy struct ceph_file_layout, but no kernel internal types.
>
> The type is defined in ceph_fs.h, which is shared/synced with the userland
> code. It's not specific to the ioctl interface.
I don't think applications should include the whole networking
protocol to use an ioctl. You should split that.
>
> My understanding (IIRC after reading comments from a recent fs merge) was
> that these sorts of headers normally shouldn't go in include/linux, since
> any userland (admin) progs will have their own version anyway while being
> built, and may not be synced with the installed kernel.
I don't think that's the normal case, no. ioctls are usually
gotten from include/linux still, although a preprocessed
version of it (mostly unifdefed and the include file has to be
especially exported)
-Andi
--
ak@xxxxxxxxxxxxxxx -- Speaking for myself only.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
