On Tue, 26 Apr 2005, David Teigland wrote:
> [Apologies, patch 1 was too large on its own.]
>
> The core dlm functions. Processes dlm_lock() and dlm_unlock() requests.
> Creates lockspaces which give applications separate contexts/namespaces in
> which to do their locking. Manages locks on resources' grant/convert/wait
> queues. Sends and receives high level locking operations between nodes.
> Delivers completion and blocking callbacks (ast's) to lock holders.
> Manages the distributed directory that tracks the current master node for
> each resource.
>
> Signed-Off-By: Dave Teigland <[email protected]>
> Signed-Off-By: Patrick Caulfield <[email protected]>
>
> +#define DLM_LOCK_IV (-1) /* invalid */
> +#define DLM_LOCK_NL (0) /* null */
> +#define DLM_LOCK_CR (1) /* concurrent read */
> +#define DLM_LOCK_CW (2) /* concurrent write */
> +#define DLM_LOCK_PR (3) /* protected read */
> +#define DLM_LOCK_PW (4) /* protected write */
> +#define DLM_LOCK_EX (5) /* exclusive */
^^^^^
|----- Why the parenthesis?
[...]
> +#define DLM_RESNAME_MAXLEN (64)
^^^^^--- more parens.
[...]
> +#define DLM_LVB_LEN (32)
^^^^^--- yet more.
[...]
> +#define DLM_LKF_NOQUEUE (0x00000001)
> +#define DLM_LKF_CANCEL (0x00000002)
> +#define DLM_LKF_CONVERT (0x00000004)
> +#define DLM_LKF_VALBLK (0x00000008)
> +#define DLM_LKF_QUECVT (0x00000010)
> +#define DLM_LKF_IVVALBLK (0x00000020)
> +#define DLM_LKF_CONVDEADLK (0x00000040)
> +#define DLM_LKF_PERSISTENT (0x00000080)
> +#define DLM_LKF_NODLCKWT (0x00000100)
> +#define DLM_LKF_NODLCKBLK (0x00000200)
> +#define DLM_LKF_EXPEDITE (0x00000400)
> +#define DLM_LKF_NOQUEUEBAST (0x00000800)
> +#define DLM_LKF_HEADQUE (0x00001000)
> +#define DLM_LKF_NOORDER (0x00002000)
> +#define DLM_LKF_ORPHAN (0x00004000)
> +#define DLM_LKF_ALTPR (0x00008000)
> +#define DLM_LKF_ALTCW (0x00010000)
^^^^^^^^^^^^
what's your facination with parenthesis?
[...]
> +#define DLM_ECANCEL (0x10001)
> +#define DLM_EUNLOCK (0x10002)
^--- here we go again.
[...]
> +#define DLM_SBF_DEMOTED (0x01)
> +#define DLM_SBF_VALNOTVALID (0x02)
> +#define DLM_SBF_ALTMODE (0x04)
^--- and again.
[...]
> +
> +struct dlm_lksb {
> + int sb_status;
> + uint32_t sb_lkid;
> + char sb_flags;
> + char * sb_lvbptr;
why not char *sb_lvbptr; ???
[...]
> +#define DLM_LOCKSPACE_LEN (64)
> +#define DLM_TOSS_SECS (10)
> +#define DLM_SCAN_SECS (5)
> +
> +#ifndef TRUE
> +#define TRUE (1)
> +#endif
> +
> +#ifndef FALSE
> +#define FALSE (0)
> +#endif
a few cases of pointless parenthesis around define values...
[...]
> +#define MAX(a, b) (((a) > (b)) ? (a) : (b))
What's wrong with the standard min/max macros from linux/kernel.h ?
[...]
> +#define DLM_ASSERT(x, do) \
> +{ \
> + if (!(x)) \
> + { \
> + printk("\nDLM: Assertion failed on line %d of file %s\n" \
^^^--- a loglevel perhaps?
[...]
> +struct dlm_args {
> + uint32_t flags;
> + void * astaddr;
void *astaddr;
> + long astparam;
> + void * bastaddr;
void *bastaddr;
> + int mode;
> + struct dlm_lksb * lksb;
struct dlm_lksb *lksb;
> + struct dlm_range * range;
struct dlm_range *range;
> +};
In other locations as well.
[...]
> +#define AST_COMP (1)
> +#define AST_BAST (2)
> +
> +/* lkb_range[] */
> +
> +#define GR_RANGE_START (0)
> +#define GR_RANGE_END (1)
> +#define RQ_RANGE_START (2)
> +#define RQ_RANGE_END (3)
> +
> +/* lkb_status */
> +
> +#define DLM_LKSTS_WAITING (1)
> +#define DLM_LKSTS_GRANTED (2)
> +#define DLM_LKSTS_CONVERT (3)
> +
> +/* lkb_flags */
> +
> +#define DLM_IFL_MSTCPY (0x00010000)
> +#define DLM_IFL_RESEND (0x00020000)
> +#define DLM_IFL_RANGE (0x00000001)
Here, again, we have a lot of pointless parenthesis around the values. I'm
not going to bother pointing out the remaining ones.
> +static int dlm_astd(void *data)
> +{
> + while (!kthread_should_stop()) {
> + set_current_state(TASK_INTERRUPTIBLE);
> + if (!test_bit(WAKE_ASTS, &astd_wakeflags))
> + schedule();
> + set_current_state(TASK_RUNNING);
> +
> + down(&astd_running);
> + if (test_and_clear_bit(WAKE_ASTS, &astd_wakeflags))
> + process_asts();
> + up(&astd_running);
> + }
> + return 0;
> +}
Always returning 0 - why not a void function then?
[...]
> +
> +int dlm_recover_directory(struct dlm_ls *ls)
> +{
> + struct dlm_member *memb;
> + struct dlm_direntry *de;
> + char *b, *last_name;
> + int error = -ENOMEM, last_len, count = 0;
Wouldn't
int error = -ENOMEM;
int last_len;
int count = 0;
be a bit more readable?
[...]
> +void dlm_lockspace_exit(void)
> +{
> +}
huh?
[...]
> +int dlm_scand(void *data)
> +{
> + struct dlm_ls *ls;
> +
> + while (!kthread_should_stop()) {
> + list_for_each_entry(ls, &lslist, ls_list)
> + dlm_scan_rsbs(ls);
> + set_current_state(TASK_INTERRUPTIBLE);
> + schedule_timeout(DLM_SCAN_SECS * HZ);
> + }
> + return 0;
> +}
void func?
[...]
> +int __init init_dlm(void)
> +{
> + int error;
> +
> + error = dlm_memory_init();
> + if (error)
> + goto out;
> +
> + error = dlm_lockspace_init();
> + if (error)
> + goto out_mem;
> +
> + error = dlm_node_ioctl_init();
> + if (error)
> + goto out_ls;
> +
> + error = dlm_member_sysfs_init();
> + if (error)
> + goto out_node;
> +
> + error = dlm_register_debugfs();
> + if (error)
> + goto out_member;
> +
> + error = dlm_lowcomms_init();
> + if (error)
> + goto out_debug;
> +
> + printk("DLM (built %s %s) installed\n", __DATE__, __TIME__);
^^^--- how about adding an explicit loglevel?
[...]
> +static inline uint32_t
> +hash_more_internal(const void *data, unsigned int len, uint32_t hash)
A lot of people prefer to have the return type on the same line as the
function name - very nice when grep'ing for a function to be able to see
the return type in the grep output.
[...]
> +void dlm_message_out(struct dlm_message *ms)
> +{
> + struct dlm_header *hd = (struct dlm_header *) ms;
> +
> + header_out(hd);
> +
> + ms->m_type = cpu_to_le32(ms->m_type);
> + ms->m_nodeid = cpu_to_le32(ms->m_nodeid);
> + ms->m_pid = cpu_to_le32(ms->m_pid);
> + ms->m_lkid = cpu_to_le32(ms->m_lkid);
> + ms->m_remid = cpu_to_le32(ms->m_remid);
> + ms->m_parent_lkid = cpu_to_le32(ms->m_parent_lkid);
> + ms->m_parent_remid = cpu_to_le32(ms->m_parent_remid);
> + ms->m_exflags = cpu_to_le32(ms->m_exflags);
> + ms->m_sbflags = cpu_to_le32(ms->m_sbflags);
> + ms->m_flags = cpu_to_le32(ms->m_flags);
> + ms->m_lvbseq = cpu_to_le32(ms->m_lvbseq);
> +
why this blank line.?
> + ms->m_status = cpu_to_le32(ms->m_status);
> + ms->m_grmode = cpu_to_le32(ms->m_grmode);
> + ms->m_rqmode = cpu_to_le32(ms->m_rqmode);
> + ms->m_bastmode = cpu_to_le32(ms->m_bastmode);
> + ms->m_asts = cpu_to_le32(ms->m_asts);
> + ms->m_result = cpu_to_le32(ms->m_result);
> +
one more blank line..
> + ms->m_range[0] = cpu_to_le64(ms->m_range[0]);
> + ms->m_range[1] = cpu_to_le64(ms->m_range[1]);
> +}
> +
> +void dlm_message_in(struct dlm_message *ms)
> +{
> + struct dlm_header *hd = (struct dlm_header *) ms;
> +
> + header_in(hd);
> +
> + ms->m_type = le32_to_cpu(ms->m_type);
> + ms->m_nodeid = le32_to_cpu(ms->m_nodeid);
> + ms->m_pid = le32_to_cpu(ms->m_pid);
> + ms->m_lkid = le32_to_cpu(ms->m_lkid);
> + ms->m_remid = le32_to_cpu(ms->m_remid);
> + ms->m_parent_lkid = le32_to_cpu(ms->m_parent_lkid);
> + ms->m_parent_remid = le32_to_cpu(ms->m_parent_remid);
> + ms->m_exflags = le32_to_cpu(ms->m_exflags);
> + ms->m_sbflags = le32_to_cpu(ms->m_sbflags);
> + ms->m_flags = le32_to_cpu(ms->m_flags);
> + ms->m_lvbseq = le32_to_cpu(ms->m_lvbseq);
> +
again a blank line...
> + ms->m_status = le32_to_cpu(ms->m_status);
> + ms->m_grmode = le32_to_cpu(ms->m_grmode);
> + ms->m_rqmode = le32_to_cpu(ms->m_rqmode);
> + ms->m_bastmode = le32_to_cpu(ms->m_bastmode);
> + ms->m_asts = le32_to_cpu(ms->m_asts);
> + ms->m_result = le32_to_cpu(ms->m_result);
> +
and one more blank line....
> + ms->m_range[0] = le64_to_cpu(ms->m_range[0]);
> + ms->m_range[1] = le64_to_cpu(ms->m_range[1]);
> +}
[...]
> --- a/drivers/dlm/util.h 1970-01-01 07:30:00.000000000 +0730
> +++ b/drivers/dlm/util.h 2005-04-25 22:52:04.199779824 +0800
> @@ -0,0 +1,23 @@
> +/******************************************************************************
> +*******************************************************************************
> +**
> +** Copyright (C) 2005 Red Hat, Inc. All rights reserved.
> +**
> +** This copyrighted material is made available to anyone wishing to use,
> +** modify, copy, or redistribute it subject to the terms and conditions
> +** of the GNU General Public License v.2.
> +**
> +*******************************************************************************
> +******************************************************************************/
> +
> +#ifndef __UTIL_DOT_H__
> +#define __UTIL_DOT_H__
> +
> +uint32_t dlm_hash(const char *data, int len);
> +
> +void dlm_message_out(struct dlm_message *ms);
> +void dlm_message_in(struct dlm_message *ms);
> +void dlm_rcom_out(struct dlm_rcom *rc);
> +void dlm_rcom_in(struct dlm_rcom *rc);
> +
> +#endif
No newline at end of file?
--
Jesper Juhl
-
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/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
