From mboxrd@z Thu Jan  1 00:00:00 1970
From: Al Viro <viro@ZenIV.linux.org.uk>
Subject: Re: updated orangefs tree at kernel.org
Date: Sat, 10 Oct 2015 21:23:48 +0100
Message-ID: <20151010202348.GG22011@ZenIV.linux.org.uk>
References: <CAOg9mSTOaEUR76NT-ZD6Msvq=aMfPPhMX6AhMc1SCxQCj_ncNA@mail.gmail.com>
 <20151008210745.GW22011@ZenIV.linux.org.uk>
 <20151008230333.GX22011@ZenIV.linux.org.uk>
 <20151010023157.GE22011@ZenIV.linux.org.uk>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>
To: Mike Marshall <hubcap@omnibond.com>
Return-path: <linux-fsdevel-owner@vger.kernel.org>
Received: from zeniv.linux.org.uk ([195.92.253.2]:33208 "EHLO
	ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1752160AbbJJUXv (ORCPT
	<rfc822;linux-fsdevel@vger.kernel.org>);
	Sat, 10 Oct 2015 16:23:51 -0400
Content-Disposition: inline
In-Reply-To: <20151010023157.GE22011@ZenIV.linux.org.uk>
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

May I politely inquire about the intended meaning of the following
definition?
#define MAX_ALIGNED_DEV_REQ_DOWNSIZE                            \
                (MAX_DEV_REQ_DOWNSIZE +                         \
                        ((((MAX_DEV_REQ_DOWNSIZE /              \
                                (BITS_PER_LONG_DIV_8)) *        \
                                (BITS_PER_LONG_DIV_8)) +        \
                            (BITS_PER_LONG_DIV_8)) -            \
                        MAX_DEV_REQ_DOWNSIZE))

1) (x + (y - x)) is more commonly spelled as y
2) ((x / y) * y + y) might not be doing what you think it is doing
3) aligning the thing to multiple of sizeof(long) is an odd thing
to do, seeing that the value being aligned is 16 + sizeof
of a structure that contains pointers (which is an atrocity in its
own right, BTW).
4) on the related note, what is the meaning of
        static int max_downsize = MAX_ALIGNED_DEV_REQ_DOWNSIZE;
        int ret = 0, num_remaining = max_downsize;
(with no code other code touching that max_downsize thing)?
5) having collected the contents of the first 4 segments of your writev()
argument and having verified that their total size does not exceed
MAX_ALIGNED_DEV_REQ_DOWNSIZE, you proceed to do this:
                payload_size -= (2 * sizeof(__s32) + sizeof(__u64));
                if (payload_size <= sizeof(struct pvfs2_downcall_s))
                        /* copy the passed in downcall into the op */
                        memcpy(&op->downcall,
                               ptr,
                               sizeof(struct pvfs2_downcall_s));
                else
                        gossip_debug(GOSSIP_DEV_DEBUG,
                                     "writev: Ignoring %d bytes\n",
                                     payload_size);
upon the entry payload_size contains the amount of data collected.  What is the
intended meaning of the 'else' branch in there?  Note that it *is* possible
to hit, exactly because MAX_ALIGNED_DEV_REQ_DOWNSIZE is sizeof(long)
greater than 16 + sizeof(struct pvfs2_downcall_s).  What should happen if we
do hit it?  I do understand the "we shall whine" part, but then what?  Leave
the op->downcall uninitialized and proceed with whatever might've been there?

That code does marshalling of userland-supplied data.  Worse, the ABI is both
undocumented and utterly... I believe "idiosyncratic" would be a printable
term for that kind of thing.  Sigh...