headers
Jeff Garzik | 3 Feb 2010 07:35
Favicon

[PATCH] Re: [PATCHv2] cld: use XDR for all messages

On 01/29/2010 08:01 PM, Colin McCabe wrote:
> That seems reasonable. All of those functions could be looked at as "common."
>
> The intention was to group together a bunch of functions that were
> useful for packet formatting. Although the bulk of the formatting is
> done by XDR, there are some things that XDR doesn't do, like
> generating and checking SHA sums. cld_dump_buf and cld_pkt_hdr_to_str
> are purely for debugging purposes, but they seemed like something that
> would be generally useful for developers making packet format changes.
> I know that they were useful to me.

I finally pushed out several changes split off from your main XDR patch, 
to the upstream cld git repo.  It took longer than expected because I 
would make changes of my own along the way, which, each time, 
necessitated a rediff+rebuild between "current cld" working tree and 
"cld + xdr" working tree.

The attached patch is what remains to be split up and committed.  I have 
definitely whittled it down, and along the way, moved and renamed some 
things on my own.  With regards to cld_fmt.*, my main objection was to 
creating too many to-be-installed headers.  One more header can be a 
pain for us and for users, while one more source file in cld/lib/ is no 
big deal.

Thus, the intention is to eliminate cld_fmt.h (newly renamed to 
cld_pkt.h) altogether, while keeping the arrangement you created in 
cld_fmt.c (newly renamed to lib/pkt.c).

I will continue whittling down the patch until it just contains the XDR 
changes themselves.
(Continue reading)

Permalink | Reply | 2 comments |
headers
Colin McCabe | 3 Feb 2010 14:29
Picon
Gravatar

[PATCH 1/2] cld: fix CLD_INODE_NAME_MAX woes

When we create a static buffer for an inode name, and treat it like a
null-terminated string, it needs to be of length CLD_INODE_NAME_MAX + 1 so
that it can hold the NULL-terminator.

In cldc_del and cldc_open, we should check that the user-submitted inode name
is less than or equal to CLD_INODE_NAME_MAX. Formerly we were just checking
that it wasn't too big to fit in the packet.

When copying the inode name out of struct cld_dirent_cur, use snprintf rather
than strcpy to ensure that we never overflow the buffer. This isn't strictly
necessary if all other checks are working perfectly, but it seems prudent.

Signed-off-by: Colin McCabe <cmccabe <at> alumni.cmu.edu>
---
 include/cldc.h |    2 +-
 lib/cldc.c     |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/cldc.h b/include/cldc.h
index f1db7d2..0d72669 100644
--- a/include/cldc.h
+++ b/include/cldc.h
 <at>  <at>  -41,7 +41,7  <at>  <at>  struct cldc_call_opts {
 			struct cld_msg_get_resp resp;
 			const char *buf;
 			unsigned int size;
-			char inode_name[CLD_INODE_NAME_MAX];
+			char inode_name[CLD_INODE_NAME_MAX + 1];
 		} get;
 	} u;
(Continue reading)

Permalink | Reply | 1 comment |
headers
Colin McCabe | 3 Feb 2010 14:29
Picon
Gravatar

[PATCH 2/2] cld: kill CLD_MAX_PKT_MSG, add CLD_MAX_PAYLOAD_SZ

Get rid of CLD_MAX_PKT_MSG. It only existed so that we could use static arrays
in a few places.

Create CLD_MAX_PAYLOAD_SZ to represent the maximum size of a message that the
API user can GET or PUT. Reducing this constant could break users who
relied on the old maximum data size, so we should try not to do it often.

Signed-off-by: Colin McCabe <cmccabe <at> alumni.cmu.edu>
---
 include/cld_msg.h |    7 ++++---
 include/cldc.h    |    4 ++--
 lib/cldc.c        |   24 +++++++++++++++++-------
 server/msg.c      |    7 +++++++
 server/session.c  |   12 ++++++++----
 tools/cldcli.c    |    2 +-
 6 files changed, 39 insertions(+), 17 deletions(-)

diff --git a/include/cld_msg.h b/include/cld_msg.h
index 6117be8..f24e4e0 100644
--- a/include/cld_msg.h
+++ b/include/cld_msg.h
 <at>  <at>  -34,9 +34,10  <at>  <at>  enum {
 	CLD_MAX_SECRET_KEY	= 128,		/**≤ includes req. nul */

 	CLD_MAX_PKT_MSG_SZ	= 1024,
-	CLD_MAX_PKT_MSG		= 128,
-	CLD_MAX_MSG_SZ		= CLD_MAX_PKT_MSG * 1024, /**≤ maximum total
-					      msg size, including all packets */
+	CLD_MAX_PAYLOAD_SZ	= 131072,	/**≤ maximum size of data that users
+						  can GET or PUT */
(Continue reading)

Permalink | Reply | 0 comments |
headers
Colin McCabe | 3 Feb 2010 22:45
Picon
Gravatar

Re: [PATCH] Re: [PATCHv2] cld: use XDR for all messages

On Tue, Feb 2, 2010 at 10:35 PM, Jeff Garzik <jeff <at> garzik.org> wrote:
> On 01/29/2010 08:01 PM, Colin McCabe wrote:
>>
>> That seems reasonable. All of those functions could be looked at as
>> "common."
>>
>> The intention was to group together a bunch of functions that were
>> useful for packet formatting. Although the bulk of the formatting is
>> done by XDR, there are some things that XDR doesn't do, like
>> generating and checking SHA sums. cld_dump_buf and cld_pkt_hdr_to_str
>> are purely for debugging purposes, but they seemed like something that
>> would be generally useful for developers making packet format changes.
>> I know that they were useful to me.
>
> I finally pushed out several changes split off from your main XDR patch, to
> the upstream cld git repo.  It took longer than expected because I would
> make changes of my own along the way, which, each time, necessitated a
> rediff+rebuild between "current cld" working tree and "cld + xdr" working
> tree.
>
> The attached patch is what remains to be split up and committed.  I have
> definitely whittled it down, and along the way, moved and renamed some
> things on my own.  With regards to cld_fmt.*, my main objection was to
> creating too many to-be-installed headers.  One more header can be a pain
> for us and for users, while one more source file in cld/lib/ is no big deal.
>
> Thus, the intention is to eliminate cld_fmt.h (newly renamed to cld_pkt.h)
> altogether, while keeping the arrangement you created in cld_fmt.c (newly
> renamed to lib/pkt.c).
(Continue reading)

Permalink | Reply | 1 comment |
headers
Colin McCabe | 3 Feb 2010 14:45
Picon
Gravatar

[PATCHv2 2/2] cld: kill CLD_MAX_PKT_MSG, add CLD_MAX_PAYLOAD_SZ

Get rid of CLD_MAX_PKT_MSG. It only existed so that we could use static arrays
in a few places.

Create CLD_MAX_PAYLOAD_SZ to represent the maximum size of a message that the
API user can GET or PUT. Reducing this constant could break users who
relied on the old maximum data size, so we should try not to do it often.

Signed-off-by: Colin McCabe <cmccabe <at> alumni.cmu.edu>
---
 include/cld_msg.h |    7 ++++---
 include/cldc.h    |    4 ++--
 lib/cldc.c        |   24 +++++++++++++++++-------
 server/msg.c      |    7 +++++++
 server/session.c  |   12 ++++++++----
 5 files changed, 38 insertions(+), 16 deletions(-)

diff --git a/include/cld_msg.h b/include/cld_msg.h
index 6117be8..f24e4e0 100644
--- a/include/cld_msg.h
+++ b/include/cld_msg.h
 <at>  <at>  -34,9 +34,10  <at>  <at>  enum {
 	CLD_MAX_SECRET_KEY	= 128,		/**≤ includes req. nul */

 	CLD_MAX_PKT_MSG_SZ	= 1024,
-	CLD_MAX_PKT_MSG		= 128,
-	CLD_MAX_MSG_SZ		= CLD_MAX_PKT_MSG * 1024, /**≤ maximum total
-					      msg size, including all packets */
+	CLD_MAX_PAYLOAD_SZ	= 131072,	/**≤ maximum size of data that users
+						  can GET or PUT */
+	CLD_MAX_MSG_SZ		= 196608,	/**≤ maximum total
(Continue reading)

Permalink | Reply | 1 comment |
headers
Colin McCabe | 3 Feb 2010 14:45
Picon
Gravatar

[PATCHv2 1/2] cld: fix CLD_INODE_NAME_MAX woes

v2: one part of this patch was originally accidentally mixed into patch 2

When we create a static buffer for an inode name, and treat it like a
null-terminated string, it needs to be of length CLD_INODE_NAME_MAX + 1 so
that it can hold the NULL-terminator.

In cldc_del and cldc_open, we should check that the user-submitted inode name
is less than or equal to CLD_INODE_NAME_MAX. Formerly we were just checking
that it wasn't too big to fit in the packet.

When copying the inode name out of struct cld_dirent_cur, use snprintf rather
than strcpy to ensure that we never overflow the buffer. This isn't strictly
necessary if all other checks are working perfectly, but it seems prudent.

Signed-off-by: Colin McCabe <cmccabe <at> alumni.cmu.edu>
---
 include/cldc.h |    2 +-
 lib/cldc.c     |    4 ++--
 tools/cldcli.c |    1 +
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/include/cldc.h b/include/cldc.h
index f1db7d2..0d72669 100644
--- a/include/cldc.h
+++ b/include/cldc.h
 <at>  <at>  -41,7 +41,7  <at>  <at>  struct cldc_call_opts {
 			struct cld_msg_get_resp resp;
 			const char *buf;
 			unsigned int size;
-			char inode_name[CLD_INODE_NAME_MAX];
(Continue reading)

Permalink | Reply | 4 comments |
headers
Jeff Garzik | 3 Feb 2010 23:27
Favicon

Re: [PATCHv2 1/2] cld: fix CLD_INODE_NAME_MAX woes

On 02/03/2010 08:45 AM, Colin McCabe wrote:
> When we create a static buffer for an inode name, and treat it like a
> null-terminated string, it needs to be of length CLD_INODE_NAME_MAX + 1 so
> that it can hold the NULL-terminator.
>
> In cldc_del and cldc_open, we should check that the user-submitted inode name
> is less than or equal to CLD_INODE_NAME_MAX. Formerly we were just checking
> that it wasn't too big to fit in the packet.
>
> When copying the inode name out of struct cld_dirent_cur, use snprintf rather
> than strcpy to ensure that we never overflow the buffer. This isn't strictly
> necessary if all other checks are working perfectly, but it seems prudent.
>
> Signed-off-by: Colin McCabe<cmccabe <at> alumni.cmu.edu>

applied, after s/snprintf/strncpy/

In general, too, you should never pass a variable string into snprintf, 
as that may make a program vulnerable to printf format string attacks 
(user supplies "%s" as a username, for example).

A few other changes made to your XDR work:

* "\n" removed from log messages, as that is appended as needed by log 
implementation

* user_key() restored.  that is our authentication hook, and it must be 
called, even though it merely returns the username passed to it at present.

* msg type renamed back to msg op
(Continue reading)

Permalink | Reply | 1 comment |
headers
Jeff Garzik | 4 Feb 2010 00:10
Favicon

Re: [Patch 4/7] tabled: retry conflicting locks

On 01/20/2010 05:56 PM, Pete Zaitcev wrote:
> Is there a way to cancel an outstanding lock request? How? You seem
> to think that there's no problem.
>
> Actually I think an cmo_close on a handle that has outstanding
> requests of any kind should drop them, so I was incorrect about
> killing the session being the only way. Maybe I can create some
> kind of ncld_open_locked() by using that feature. That ought to
> be good enough.

CLOSE always removes outstanding locks, FWIW...  always has.

	Jeff
Permalink | Reply | 0 comments |
headers
Jeff Garzik | 4 Feb 2010 00:20
Favicon

[PATCH] Re: [PATCHv2 2/2] cld: kill CLD_MAX_PKT_MSG, add CLD_MAX_PAYLOAD_SZ

On 02/03/2010 08:45 AM, Colin McCabe wrote:
> Get rid of CLD_MAX_PKT_MSG. It only existed so that we could use static arrays
> in a few places.
>
> Create CLD_MAX_PAYLOAD_SZ to represent the maximum size of a message that the
> API user can GET or PUT. Reducing this constant could break users who
> relied on the old maximum data size, so we should try not to do it often.
>
> Signed-off-by: Colin McCabe<cmccabe <at> alumni.cmu.edu>
> ---
>   include/cld_msg.h |    7 ++++---
>   include/cldc.h    |    4 ++--
>   lib/cldc.c        |   24 +++++++++++++++++-------
>   server/msg.c      |    7 +++++++
>   server/session.c  |   12 ++++++++----
>   5 files changed, 38 insertions(+), 16 deletions(-)

applied

I've attached the latest cld->cld.rpcgen working diff, current as of commit

commit a592ee77012f3c32d775c1e67fff66c944e7b5fe
Author: Colin McCabe <cmccabe <at> alumni.cmu.edu>
Date:   Wed Feb 3 18:17:24 2010 -0500

     libcldc: use typed message completion callback

     Signed-off-by: Colin McCabe <cmccabe <at> alumni.cmu.edu>
     Signed-off-by: Jeff Garzik <jgarzik <at> redhat.com>
(Continue reading)

Permalink | Reply | 0 comments |
headers
Colin McCabe | 4 Feb 2010 02:21
Picon
Gravatar

Re: [PATCHv2 1/2] cld: fix CLD_INODE_NAME_MAX woes

On Wed, Feb 3, 2010 at 2:27 PM, Jeff Garzik <jeff <at> garzik.org> wrote:
> On 02/03/2010 08:45 AM, Colin McCabe wrote:
>>
>> When we create a static buffer for an inode name, and treat it like a
>> null-terminated string, it needs to be of length CLD_INODE_NAME_MAX + 1 so
>> that it can hold the NULL-terminator.
>>
>> In cldc_del and cldc_open, we should check that the user-submitted inode
>> name
>> is less than or equal to CLD_INODE_NAME_MAX. Formerly we were just
>> checking
>> that it wasn't too big to fit in the packet.
>>
>> When copying the inode name out of struct cld_dirent_cur, use snprintf
>> rather
>> than strcpy to ensure that we never overflow the buffer. This isn't
>> strictly
>> necessary if all other checks are working perfectly, but it seems prudent.
>>
>> Signed-off-by: Colin McCabe<cmccabe <at> alumni.cmu.edu>
>
> applied, after s/snprintf/strncpy/
>
> In general, too, you should never pass a variable string into snprintf, as
> that may make a program vulnerable to printf format string attacks (user
> supplies "%s" as a username, for example).

Oh, how embarassing. I hate it when people do that, now I did it too.

I tend to avoid strncpy because of how it always writes n bytes. If
(Continue reading)

Permalink | Reply | 0 comments |

Gmane