On 10/03/2002 at 9:50 AM, Christoph Hellwig wrote:
>> +/**
>> + * helpful PROCFS macro
>> + **/
>> +#ifdef CONFIG_PROC_FS
>> +#define PROCPRINT(msg, args...) (sz += sprintf(page + sz, msg, ##
args));\
>> + if (sz < off)\
>> + off -= sz, sz = 0;\
>> + else if (sz >= off + count)\
>> + goto out
>> +#endif
> I think this really wants to be converted to the seq_file interface..
We plan to.
>> +
>> +/**
>> + * PluginID convenience macros
>> + *
>> + * An EVMS PluginID is a 32-bit number with the following bit
positions:
>> + * Top 16 bits: OEM identifier. See IBM_OEM_ID.
>> + * Next 4 bits: Plugin type identifier. See evms_plugin_code.
>> + * Lowest 12 bits: Individual plugin identifier within a given plugin
type.
>> + **/
>> +#define SetPluginID(oem, type, id) ((oem << 16) | (type << 12) | id)
>> +#define GetPluginOEM(pluginid) (pluginid >> 16)
>> +#define GetPluginType(pluginid) ((pluginid >> 12) & 0xf)
>> +#define GetPluginID(pluginid) (pluginid & 0xfff)
> What is the prupose of OEM IDs?
To allow unique identification of plugins. If you wrote
plugin, you might give it an ID of 1. Someone else
may do the same thing. However, by letting you add
something specific which identifies you (i.e. like
your initials), then possibility of collisions is
reduced.
>> +struct evms_plugin_header {
>> + u32 id;
>> + struct evms_version version;
>> + struct evms_version required_services_version;
>> + struct evms_plugin_fops *fops;
>> + struct list_head headers;
>> +};
> What is the required services version?
The common services are a set of functions exported
by the core code. We have major,minor,patchlevel
versions for them. Plugin writers specify the
version of the interface they are coded to comply
with. Mismatching core services and plugin
version expectations are caught at plugin registration
(load) time, and prevented from being usable.
>> +struct evms_plugin_fops {
> What about evms_plugin_ops?
>> + int (*ioctl) (struct evms_logical_node *, struct inode *,
>> + struct file *, u32, unsigned long);
>> + int (*direct_ioctl) (struct inode *, struct file *,
>> + u32, unsigned long);
> Umm, why do you need two ioctl handlers?
the IOCTL entry point is used to send to volumes.
the DIRECT_IOCTL entry point is used for point-
to-point ioctls between corresponding user space
and kernel space plugins.
>> +/**
>> + * convenience macros to use plugin's fops entry points
>> + **/
>> +#define DISCOVER(node, list) ((plugin)->fops->discover(list))
>> +#define END_DISCOVER(node, list) ((plugin)->fops->end_discover(list))
>> +#define DELETE(node) ((node)->plugin->fops->delete(node))
>> +#define SUBMIT_IO(node, bio) ((node)->plugin->fops->submit_io(node,
bio))
>> +#define INIT_IO(node, rw_flag, start_sec, num_secs, buf_addr) >((node)
->plugin->fops->init_io(node, rw_flag, start_sec, num_secs, buf_addr))
>> +#define IOCTL(node, inode, file, cmd, arg) ((node)
->plugin->fops->ioctl(node, >inode, file, cmd, arg))
>> +#define DIRECT_IOCTL(plugin, inode, file, cmd, arg) >((plugin)
->fops->direct_ioctl(inode, file, cmd, arg))
> Do you really need those wrapper?
No the wrappers aren't really needed. However they do make the
code a great deal more readable.
> They just obsfucate the code
The same argument could be made about *all* macros then.
Its simply a tradeoff between readability and potential
hiding.
>> +/**
>> + * struct evms_pool_mgmt - anchor block for private pool management
>> + * @cachep: kmem_cache_t variable
>> + * @member_size: size of each element in the pool
>> + * @head:
>> + * @waiters: count of waiters
>> + * @wait_queue: list of waiters
>> + * @name: name of the pool (must be less than 20 chars)
>> + *
>> + * anchor block for private pool management
>> + **/
>> +struct evms_pool_mgmt {
>> + kmem_cache_t *cachep;
>> + int member_size;
>> + void *head;
>> + atomic_t waiters;
>> + wait_queue_head_t wait_queue;
>> + u8 *name;
>> +};
> What's the pruipose of this? Is it really evms-specific?
Its a reminent of 2.4 stuff before mempool was available.
Its gone.
>> +
>> +/*
>> + * Notes:
>> + * All of the following kernel thread functions belong to EVMS
base.
>> + * These functions were copied from md_core.c
>> + */
> What about moving them to the core kernel code so everyone
> can use them?
I've got no problem with doing that.
>> +/* Have to include this at the end, since it depends
>> + * on structures and definitions in this file.
>> + */
>> +#include <linux/evms/evms_ioctl.h>
> Just remove this and make the individual sources include it
Sure, its easy to do. Having nested includes allowed us
to enforce include ordering.
Mark
> >> +#define SetPluginID(oem, type, id) ((oem << 16) | (type << 12) | id)
> >> +#define GetPluginOEM(pluginid) (pluginid >> 16)
> >> +#define GetPluginType(pluginid) ((pluginid >> 12) & 0xf)
> >> +#define GetPluginID(pluginid) (pluginid & 0xfff)
>
> > What is the prupose of OEM IDs?
>
> To allow unique identification of plugins. If you wrote
> plugin, you might give it an ID of 1. Someone else
> may do the same thing. However, by letting you add
> something specific which identifies you (i.e. like
> your initials), then possibility of collisions is
> reduced.
Please stop using magic numbers in the kernel _at all_. The only
sensible thing against collision is proper naming, i.e. strings.
>
> >> +struct evms_plugin_header {
> >> + u32 id;
> >> + struct evms_version version;
> >> + struct evms_version required_services_version;
> >> + struct evms_plugin_fops *fops;
> >> + struct list_head headers;
> >> +};
>
> > What is the required services version?
>
> The common services are a set of functions exported
> by the core code. We have major,minor,patchlevel
> versions for them. Plugin writers specify the
> version of the interface they are coded to comply
> with. Mismatching core services and plugin
> version expectations are caught at plugin registration
> (load) time, and prevented from being usable.
Doesn't work in a linux enviroment. People just have to stick to the
kernel version they write for. If you think you really need it make
such checks at the cpp level as there is no such thing as binary
compatiblity anyway.
> the IOCTL entry point is used to send to volumes.
> the DIRECT_IOCTL entry point is used for point-
> to-point ioctls between corresponding user space
> and kernel space plugins.
Do the ioctl directly to the device node of the lower layer plugin instead.
>
> >> +/**
> >> + * convenience macros to use plugin's fops entry points
> >> + **/
> >> +#define DISCOVER(node, list) ((plugin)->fops->discover(list))
> >> +#define END_DISCOVER(node, list) ((plugin)->fops->end_discover(list))
> >> +#define DELETE(node) ((node)->plugin->fops->delete(node))
> >> +#define SUBMIT_IO(node, bio) ((node)->plugin->fops->submit_io(node,
> bio))
> >> +#define INIT_IO(node, rw_flag, start_sec, num_secs, buf_addr) >((node)
> ->plugin->fops->init_io(node, rw_flag, start_sec, num_secs, buf_addr))
> >> +#define IOCTL(node, inode, file, cmd, arg) ((node)
> ->plugin->fops->ioctl(node, >inode, file, cmd, arg))
> >> +#define DIRECT_IOCTL(plugin, inode, file, cmd, arg) >((plugin)
> ->fops->direct_ioctl(inode, file, cmd, arg))
>
> > Do you really need those wrapper?
>
> No the wrappers aren't really needed. However they do make the
> code a great deal more readable.
>
> > They just obsfucate the code
>
> The same argument could be made about *all* macros then.
> Its simply a tradeoff between readability and potential
> hiding.
CodingStyle is one big flamewar. As no other operations vector
in the linux kernel uses such wrappers the syle police seems to
be on my side in this case :)
On Oct 04, 2002 15:14 +0100, Christoph Hellwig wrote:
> > the IOCTL entry point is used to send to volumes.
> > the DIRECT_IOCTL entry point is used for point-
> > to-point ioctls between corresponding user space
> > and kernel space plugins.
>
> Do the ioctl directly to the device node of the lower layer plugin instead.
Not possible - EVMS doesn't export the lower-level device nodes at all.
That is one of the benefits - you can take 1000 drives and stack them
and raid and LVM them all you want, and you don't consume 1000*layers
device nodes.
> > >> +/**
> > >> + * convenience macros to use plugin's fops entry points
> > >> + **/
> > >> +#define DISCOVER(node, list) ((plugin)->fops->discover(list))
> > >> +#define END_DISCOVER(node, list) ((plugin)->fops->end_discover(list))
> > >> +#define DELETE(node) ((node)->plugin->fops->delete(node))
> > >> +#define SUBMIT_IO(node, bio) ((node)->plugin->fops->submit_io(node,
> > bio))
> > >> +#define INIT_IO(node, rw_flag, start_sec, num_secs, buf_addr) >((node)
> > ->plugin->fops->init_io(node, rw_flag, start_sec, num_secs, buf_addr))
> > >> +#define IOCTL(node, inode, file, cmd, arg) ((node)
> > ->plugin->fops->ioctl(node, >inode, file, cmd, arg))
> > >> +#define DIRECT_IOCTL(plugin, inode, file, cmd, arg) >((plugin)
> > ->fops->direct_ioctl(inode, file, cmd, arg))
> >
> > > Do you really need those wrapper?
> >
> > No the wrappers aren't really needed. However they do make the
> > code a great deal more readable.
> >
> > > They just obsfucate the code
> >
> > The same argument could be made about *all* macros then.
> > Its simply a tradeoff between readability and potential
> > hiding.
>
> CodingStyle is one big flamewar. As no other operations vector
> in the linux kernel uses such wrappers the syle police seems to
> be on my side in this case :)
Um, how about EXT3_I() and EXT3_SB(), or almost any filesystem in
2.5 which hides inode->u.generic_ip->foo_inode_info->blah? Given
that you want to keep lines < 80 chars where possible having short
names to access common methods is a big win, IMHO.
Are you telling me XFS doesn't have some awful abstractions in it too?
VOP_GETATTR(), ugh. So, yes it's nice to have useful criticism, but
no need to pick every space and tab to death.
Cheers, Andreas
--
Andreas Dilger
http://www-mddsp.enel.ucalgary.ca/People/adilger/
http://sourceforge.net/projects/ext2resize/
On Fri, Oct 04, 2002 at 08:34:02AM -0600, Andreas Dilger wrote:
> On Oct 04, 2002 15:14 +0100, Christoph Hellwig wrote:
> > > the IOCTL entry point is used to send to volumes.
> > > the DIRECT_IOCTL entry point is used for point-
> > > to-point ioctls between corresponding user space
> > > and kernel space plugins.
> >
> > Do the ioctl directly to the device node of the lower layer plugin instead.
>
> Not possible - EVMS doesn't export the lower-level device nodes at all.
> That is one of the benefits - you can take 1000 drives and stack them
> and raid and LVM them all you want, and you don't consume 1000*layers
> device nodes.
I don't think it's a benefit but really ugly. There is no reason to now
allow access to the lower layers. How do I e.g. write a new volume label to
the lower level devices?
> Um, how about EXT3_I() and EXT3_SB(), or almost any filesystem in
> 2.5 which hides inode->u.generic_ip->foo_inode_info->blah?
That one actually provides a benfit as we have two different 24 and one 2.5 method
to access it. I'm speaking about the wrappers for function pointer
invocations. And yes, XFS has some massive macro abuse, but it's legacy
code that's not to easy to change while EVMS is new, from-the-scratch code that
should rather do it right.