linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [Evms-devel] Re: [PATCH] EVMS core 2/4: evms.h
@ 2002-10-04 18:45 Steve Pratt
  2002-10-07 17:25 ` Christoph Hellwig
  0 siblings, 1 reply; 15+ messages in thread
From: Steve Pratt @ 2002-10-04 18:45 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Mark Peloquin, torvalds, linux-kernel, evms-devel


Christoph Hellwig wrote:
>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?

I am not sure I understand your question.  Did you mean that there does not
appear to be a **way** to access lower level devices or did you really mean
no reason to do so?  The reason to do so is for plug-ins such as MD which
may want to kick an array member out of the array which is a command
specific to MD.  The way in which this is accomplished in EVMS is as Mark
described above through the use of the DIRECT_IOCTL which allows for direct
communication to intermediate levels of an EVMS volume stack.

Note that this method is only used for certain cases like the MD one
described.  For normal IO type operations such as writing metadata or
changing volume labels, since EVMS in userspace knows about ALL of the
layers of the volume stack we can resolve all metadata writes to disk
relative offsets in our userspace config tools and thus don't have to write
to intermediate nodes to build more complex volumes.

Steve




^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Evms-devel] Re: [PATCH] EVMS core 2/4: evms.h
  2002-10-04 18:45 [Evms-devel] Re: [PATCH] EVMS core 2/4: evms.h Steve Pratt
@ 2002-10-07 17:25 ` Christoph Hellwig
  0 siblings, 0 replies; 15+ messages in thread
From: Christoph Hellwig @ 2002-10-07 17:25 UTC (permalink / raw)
  To: Steve Pratt; +Cc: Mark Peloquin, torvalds, linux-kernel, evms-devel

> >> 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

sorry, that "now" should have been a 'not'.

> >allow access to the lower layers.  How do I e.g. write a new volume label
> to
> >the lower level devices?
> 
> I am not sure I understand your question.  Did you mean that there does not
> appear to be a **way** to access lower level devices or did you really mean
> no reason to do so?

Sorry again for above typo - it makes my whole statement worthless..

To clarify: I think not having device nodes for anything but the uppermost
layer of evms volumes is a bad idea.  This does not only make it impossible
to access those normally from userspace but also makes evms duplicate
code in the block layers as we already have stacking support there.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Evms-devel] Re: [PATCH] EVMS core 2/4: evms.h
  2002-10-04 14:34   ` Andreas Dilger
@ 2002-10-04 17:10     ` Christoph Hellwig
  0 siblings, 0 replies; 15+ messages in thread
From: Christoph Hellwig @ 2002-10-04 17:10 UTC (permalink / raw)
  To: Mark Peloquin, torvalds, linux-kernel, evms-devel

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.


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Evms-devel] Re: [PATCH] EVMS core 2/4: evms.h
@ 2002-10-04 14:59 Mark Peloquin
  0 siblings, 0 replies; 15+ messages in thread
From: Mark Peloquin @ 2002-10-04 14:59 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: torvalds, linux-kernel, evms-devel


On 10/04/2002 at 9:08 AM, Christoph Hellwig wrote:
> You can"t know where devfs is mounted.  So of the above only
EVMS_DIR_NAME
> and EVMS_DEV_NAME make sense.

Agreed.



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Evms-devel] Re: [PATCH] EVMS core 2/4: evms.h
  2002-10-04 14:08 ` Christoph Hellwig
@ 2002-10-04 14:36   ` Richard B. Johnson
  0 siblings, 0 replies; 15+ messages in thread
From: Richard B. Johnson @ 2002-10-04 14:36 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Mark Peloquin, torvalds, linux-kernel, evms-devel

On Fri, 4 Oct 2002, Christoph Hellwig wrote:

> On Thu, Oct 03, 2002 at 12:42:23PM -0500, Mark Peloquin wrote:
> > >> +#define TRUE                            1
> > 
> > > Please just use 0/1 directly just like everyone else..
> > 
> > More than happy to comply, however grep'ing the tree its
> > plain to see that not "everyone else" is following this
> > suggestion.
> 
> Sure there are other offenders in the drivers, from known offenders like
> Richard or IBM, but no core code.  There is a reason why theses defines are
> not in kernel.h..

Is it because TRUE is not ~FALSE? or because FALSE is not ~TRUE or
is it because TRUE is not !FALSE? or because FALSE is not !TRUE?

		^;)

Cheers,
Dick Johnson
Penguin : Linux version 2.4.18 on an i686 machine (797.90 BogoMips).
The US military has given us many words, FUBAR, SNAFU, now ENRON.
Yes, top management were graduates of West Point and Annapolis.


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Evms-devel] Re: [PATCH] EVMS core 2/4: evms.h
  2002-10-04 14:14 ` Christoph Hellwig
@ 2002-10-04 14:34   ` Andreas Dilger
  2002-10-04 17:10     ` Christoph Hellwig
  0 siblings, 1 reply; 15+ messages in thread
From: Andreas Dilger @ 2002-10-04 14:34 UTC (permalink / raw)
  To: Christoph Hellwig, Mark Peloquin, torvalds, linux-kernel, evms-devel

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/


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Evms-devel] Re: [PATCH] EVMS core 2/4: evms.h
  2002-10-03 18:59 Mark Peloquin
@ 2002-10-04 14:14 ` Christoph Hellwig
  2002-10-04 14:34   ` Andreas Dilger
  0 siblings, 1 reply; 15+ messages in thread
From: Christoph Hellwig @ 2002-10-04 14:14 UTC (permalink / raw)
  To: Mark Peloquin; +Cc: torvalds, linux-kernel, evms-devel

> >> +#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 :)


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Evms-devel] Re: [PATCH] EVMS core 2/4: evms.h
  2002-10-03 17:42 Mark Peloquin
  2002-10-04 12:28 ` Robert Varga
@ 2002-10-04 14:08 ` Christoph Hellwig
  2002-10-04 14:36   ` Richard B. Johnson
  1 sibling, 1 reply; 15+ messages in thread
From: Christoph Hellwig @ 2002-10-04 14:08 UTC (permalink / raw)
  To: Mark Peloquin; +Cc: torvalds, linux-kernel, evms-devel

On Thu, Oct 03, 2002 at 12:42:23PM -0500, Mark Peloquin wrote:
> >> +#define TRUE                            1
> 
> > Please just use 0/1 directly just like everyone else..
> 
> More than happy to comply, however grep'ing the tree its
> plain to see that not "everyone else" is following this
> suggestion.

Sure there are other offenders in the drivers, from known offenders like
Richard or IBM, but no core code.  There is a reason why theses defines are
not in kernel.h..

> 
> >> +#define DEV_PATH                "/dev"
> >> +#define EVMS_DIR_NAME                 "evms"
> >> +#define EVMS_DEV_NAME                 "block_device"
> >> +#define EVMS_DEV_NODE_PATH            DEV_PATH "/" EVMS_DIR_NAME "/"
> >> +#define EVMS_DEVICE_NAME        DEV_PATH "/" EVMS_DIR_NAME "/"
> EVMS_DEV_NAME
> 
> > The kernel doesn't know about device names at all.
> 
> I realize this is a goal, and I'm not opposed to it. However,
> I know devfs is not popular, but people are using it, and it
> *is* still available in 2.5. For the cases where ppl are
> using it, the EVMS kernel component needs this info to tell
> devfs the name of the devnode to create. I don't want to get
> into a devfs flamewar, EVMS is simply offering interoperability
> with what ppl n do today. Should that change, EVMS is more
> than happy to adapt to the latest technology.

You can"t know where devfs is mounted.  So of the above only EVMS_DIR_NAME
and EVMS_DEV_NAME make sense.


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Evms-devel] Re: [PATCH] EVMS core 2/4: evms.h
@ 2002-10-04 13:59 Mark Peloquin
  0 siblings, 0 replies; 15+ messages in thread
From: Mark Peloquin @ 2002-10-04 13:59 UTC (permalink / raw)
  To: Robert Varga; +Cc: linux-kernel


On 10/04/2002 at 7:28 AM, Robert Varga wrote:
<snip...>
> Possibly shortened to:

> static inline int list_member(struct list_head *member)
> {
>     return member->next && member->prev;
> }

> Faster, and (at least to me) it looks more obvious.

Yes, this may be shorter. However with this change
the return type would also need to be changed to
portable across archs.

Mark



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Evms-devel] Re: [PATCH] EVMS core 2/4: evms.h
  2002-10-03 17:42 Mark Peloquin
@ 2002-10-04 12:28 ` Robert Varga
  2002-10-04 14:08 ` Christoph Hellwig
  1 sibling, 0 replies; 15+ messages in thread
From: Robert Varga @ 2002-10-04 12:28 UTC (permalink / raw)
  To: Mark Peloquin; +Cc: linux-kernel

On Thu, Oct 03, 2002 at 12:42:23PM -0500, Mark Peloquin wrote:
> >> +static inline int
> >> +list_member(struct list_head *member)
> >> +{
> >> +  return ((!member->next || !member->prev) ? FALSE : TRUE);
> >> +}
> 
> > This should go into list.h
> 
> Yes it should. I will pull this out of this header
> and submit a patch for list.h.

Possibly shortened to:

static inline int list_member(struct list_head *member)
{
	return member->next && member->prev;
}

Faster, and (at least to me) it looks more obvious.

-- 
Kind regards,
Robert Varga
------------------------------------------------------------------------------
n@hq.sk                                          http://hq.sk/~nite/gpgkey.txt

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Evms-devel] Re: [PATCH] EVMS core 2/4: evms.h
@ 2002-10-03 18:59 Mark Peloquin
  2002-10-04 14:14 ` Christoph Hellwig
  0 siblings, 1 reply; 15+ messages in thread
From: Mark Peloquin @ 2002-10-03 18:59 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: torvalds, linux-kernel, evms-devel


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



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Evms-devel] Re: [PATCH] EVMS core 2/4: evms.h
@ 2002-10-03 17:42 Mark Peloquin
  2002-10-04 12:28 ` Robert Varga
  2002-10-04 14:08 ` Christoph Hellwig
  0 siblings, 2 replies; 15+ messages in thread
From: Mark Peloquin @ 2002-10-03 17:42 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: torvalds, linux-kernel, evms-devel


On 10/03/2002 at 9:50 AM, Chrisopth Hellwig wrote:
>> +/* -*- linux-c -*- */
>> +/*
>> + *   Copyright (c) International Business Machines  Corp., 2000

> Has this file _really_ not been touched for two years??

Yup, it has, I've fixed them.

>> +/*
>> + * linux/include/linux/evms/evms.h
>> + *
>> + * EVMS kernel header file
>> + *
>> + */

> This comment sais exactly nothing.  You aswell just remove it..

Agreed. The comment is obvious in this case. Its gone.

>> +
>> +#ifndef __EVMS_INCLUDED__
>> +#define __EVMS_INCLUDED__
>> +
>> +#include <linux/blk.h>
>> +#include <linux/genhd.h>
>> +#include <linux/fs.h>
>> +#include <linux/iobuf.h>

> I can't see the need for this include anywhere in the file.  Please check
> whether you need all these includes.

Ok, done.

>> +
>> +/**
>> + * general defines section
>> + **/
>> +#define FALSE                           0
>> +#define TRUE                            1

> Please just use 0/1 directly just like everyone else..

More than happy to comply, however grep'ing the tree its
plain to see that not "everyone else" is following this
suggestion.

>> +#define DEV_PATH                "/dev"
>> +#define EVMS_DIR_NAME                 "evms"
>> +#define EVMS_DEV_NAME                 "block_device"
>> +#define EVMS_DEV_NODE_PATH            DEV_PATH "/" EVMS_DIR_NAME "/"
>> +#define EVMS_DEVICE_NAME        DEV_PATH "/" EVMS_DIR_NAME "/"
EVMS_DEV_NAME

> The kernel doesn't know about device names at all.

I realize this is a goal, and I'm not opposed to it. However,
I know devfs is not popular, but people are using it, and it
*is* still available in 2.5. For the cases where ppl are
using it, the EVMS kernel component needs this info to tell
devfs the name of the devnode to create. I don't want to get
into a devfs flamewar, EVMS is simply offering interoperability
with what ppl can do today. Should that change, EVMS is more
than happy to adapt to the latest technology.

>> +
>> +/**
>> + * list_for_each_entry_safe -   iterate over list safe against removal
of list entry
>> + * @pos:        the type * to use as a loop counter.
>> + * @n:              another type * to use as temporary storage
>> + * @head:       the head for your list.
>> + * @member:     the name of the list_struct within the struct.
>> + */
>> +#define list_for_each_entry_safe(pos, n, head, member)
\
>> +        for (pos = list_entry((head)->next, typeof(*pos), member),
\
>> +            n = list_entry(pos->member.next, typeof(*pos), member); \
>> +        &pos->member != (head);                             \
>> +        pos = n,                                                    \
>> +            n = list_entry(pos->member.next, typeof(*pos), member))
>> +/**
>> + * list_member - tests whether a list member is currently on a list
>> + * @member:   member to evaulate
>> + */
>> +static inline int
>> +list_member(struct list_head *member)
>> +{
>> +  return ((!member->next || !member->prev) ? FALSE : TRUE);
>> +}

> This should go into list.h

Yes it should. I will pull this out of this header
and submit a patch for list.h.

>> +
>> +/**
>> + * kernel logging levels defines
>> + **/
>> +#define EVMS_INFO_CRITICAL              0
>> +#define EVMS_INFO_SERIOUS               1
>> +#define EVMS_INFO_ERROR                 2
>> +#define EVMS_INFO_WARNING               3
>> +#define EVMS_INFO_DEFAULT               5
>> +#define EVMS_INFO_DETAILS               6
>> +#define EVMS_INFO_DEBUG                 7
>> +#define EVMS_INFO_EXTRA                 8
>> +#define EVMS_INFO_ENTRY_EXIT            9
>> +#define EVMS_INFO_EVERYTHING            10

> What about just using normal logging levels?

The quick answer is granularity. EVMS can generate a *lot*
of logging messages and we wanted a way to control the
amount of debugging messages. A simply ON/OFF didn't
quite seem adequate for many cases.

More to follow ....

Mark



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Evms-devel] Re: [PATCH] EVMS core 2/4: evms.h
  2002-10-03 15:14     ` Christoph Hellwig
@ 2002-10-03 16:22       ` Linus Torvalds
  0 siblings, 0 replies; 15+ messages in thread
From: Linus Torvalds @ 2002-10-03 16:22 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Michael Clark, Kevin Corry, linux-kernel, evms-devel


On Thu, 3 Oct 2002, Christoph Hellwig wrote:
> 
> root device should be in do_mount.c and not in obscure headers.

No, they should _not_ be in do_mount.c either. They should be in the 
driver registration, and do_mount.c should not have a random list of 
devices. 

I'm not accepting do_mount.c expansion here, simply because I don't want 
to help a horribly broken interface. You can always use a hex number 
(which is what things like lilo will install anyway, I believe, rather 
than using the "root=/dev/xxx" command line), and if people get too tired 
about remembering numbers, maybe somebody who cares will step up to the 
plate and write a reverse of "__bdevname()" and do it right.

Hint: see __bdevname in fs/block_dev.c, and realize that it does the 
"kdev->name" translation without _any_ tables at all. Think about doing 
the same the other way, by just walking the registered block devices.

		Linus


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Evms-devel] Re: [PATCH] EVMS core 2/4: evms.h
  2002-10-03 15:10   ` [Evms-devel] " Michael Clark
@ 2002-10-03 15:14     ` Christoph Hellwig
  2002-10-03 16:22       ` Linus Torvalds
  0 siblings, 1 reply; 15+ messages in thread
From: Christoph Hellwig @ 2002-10-03 15:14 UTC (permalink / raw)
  To: Michael Clark
  Cc: Christoph Hellwig, Kevin Corry, torvalds, linux-kernel, evms-devel

On Thu, Oct 03, 2002 at 11:10:14PM +0800, Michael Clark wrote:
> 
> >>+#define DEV_PATH			"/dev"
> >>+#define EVMS_DIR_NAME			"evms"
> >>+#define EVMS_DEV_NAME			"block_device"
> >>+#define EVMS_DEV_NODE_PATH		DEV_PATH "/" EVMS_DIR_NAME "/"
> >>+#define EVMS_DEVICE_NAME		DEV_PATH "/" EVMS_DIR_NAME "/" EVMS_DEV_NAME
> > 
> > 
> > The kernel doesn't know about device names at all.
> 
> It does for specifying root devices and for devfs.A

root device should be in do_mount.c and not in obscure headers.
devfs doesn'T need hardcoded "/dev" prefixes, and it's better to not
add defines for the strings in devfs so that crap doesn't get spread
over the code but is localized to the existing devfs damage

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Evms-devel] Re: [PATCH] EVMS core 2/4: evms.h
  2002-10-03 14:50 ` Christoph Hellwig
@ 2002-10-03 15:10   ` Michael Clark
  2002-10-03 15:14     ` Christoph Hellwig
  0 siblings, 1 reply; 15+ messages in thread
From: Michael Clark @ 2002-10-03 15:10 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Kevin Corry, torvalds, linux-kernel, evms-devel


>>+#define DEV_PATH			"/dev"
>>+#define EVMS_DIR_NAME			"evms"
>>+#define EVMS_DEV_NAME			"block_device"
>>+#define EVMS_DEV_NODE_PATH		DEV_PATH "/" EVMS_DIR_NAME "/"
>>+#define EVMS_DEVICE_NAME		DEV_PATH "/" EVMS_DIR_NAME "/" EVMS_DEV_NAME
> 
> 
> The kernel doesn't know about device names at all.

It does for specifying root devices and for devfs.


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2002-10-07 17:20 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2002-10-04 18:45 [Evms-devel] Re: [PATCH] EVMS core 2/4: evms.h Steve Pratt
2002-10-07 17:25 ` Christoph Hellwig
  -- strict thread matches above, loose matches on Subject: below --
2002-10-04 14:59 Mark Peloquin
2002-10-04 13:59 Mark Peloquin
2002-10-03 18:59 Mark Peloquin
2002-10-04 14:14 ` Christoph Hellwig
2002-10-04 14:34   ` Andreas Dilger
2002-10-04 17:10     ` Christoph Hellwig
2002-10-03 17:42 Mark Peloquin
2002-10-04 12:28 ` Robert Varga
2002-10-04 14:08 ` Christoph Hellwig
2002-10-04 14:36   ` Richard B. Johnson
2002-10-03 12:36 Kevin Corry
2002-10-03 14:50 ` Christoph Hellwig
2002-10-03 15:10   ` [Evms-devel] " Michael Clark
2002-10-03 15:14     ` Christoph Hellwig
2002-10-03 16:22       ` Linus Torvalds

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).