Xen devel - Oct 2013 - [PATCH] oxenstored to allow updates regardless of quota

Here I''m submitting to xen-devel a patch written by Zheng Li.

Thomas Sanders



CA-108294: allow a domain updating existing xenstore keys even if it has already
reached its max entries limit

As updating existing key won''t increase the number of entries belonging
to a domain, we should avoid checking the max entries limit prematurely. The
patch addresses this issue in the following functions: write/add, mkdir,
setperms.

Signed-off-by: Zheng Li <zheng.li@eu.citrix.com>

diff --git a/tools/ocaml/xenstored/store.ml b/tools/ocaml/xenstored/store.ml
--- a/tools/ocaml/xenstored/store.ml
+++ b/tools/ocaml/xenstored/store.ml
@@ -180,12 +180,12 @@ let get_node rnode path  		try Some (lookup_get rnode
path) with Define.Doesnt_exist -> None
 	)
 
-(* get the deepest existing node for this path *)
+(* get the deepest existing node for this path, return the node and a flag on
the existence of the full path *)
 let rec get_deepest_existing_node node = function
-	| [] -> node
+	| [] -> node, true
 	| h :: t ->
 		try get_deepest_existing_node (Node.find node h) t 
-		with Not_found -> node
+		with Not_found -> node, false
 
 let set_node rnode path nnode  	let quota = Quota.create () in
@@ -348,7 +348,7 @@ let traversal root_node f  		List.iter (_traversal (path @ [
Symbol.to_string node.Node.name ])) node.Node.children
 		in
 	_traversal [] root_node
-		
+
 let dump_store_buf root_node  	let buf = Buffer.create 8192 in
 	let dump_node path node @@ -381,16 +381,24 @@ let set_node store path node  
Quota.add store.quota quota_diff
 
 let write store perm path value -	let owner = Node.get_owner
(get_deepest_existing_node store path) in
-	Quota.check store.quota owner (String.length value);
+	let node, existing = get_deepest_existing_node store path in
+	let owner = Node.get_owner node in
+	if existing then
+		(* Only check the string length limit *)
+		Quota.check store.quota (-1) (String.length value)
+	else
+		(* Check the domain entries limit too *)
+		Quota.check store.quota owner (String.length value);
 	let root, node_created = path_write store perm path value in
 	store.root <- root;
 	if node_created
 	then Quota.add_entry store.quota owner
 
 let mkdir store perm path -	let owner = Node.get_owner
(get_deepest_existing_node store path) in
-	Quota.check store.quota owner 0;
+	let node, existing = get_deepest_existing_node store path in
+	let owner = Node.get_owner node in
+	(* It''s upt to the mkdir logic to decide what to do with existing
path *)
+	if not existing then Quota.check store.quota owner 0;
 	store.root <- path_mkdir store perm path;
 	Quota.add_entry store.quota owner
 
@@ -408,7 +416,7 @@ let setperms store perm path nperms  	| Some node ->
 		let old_owner = Node.get_owner node in
 		let new_owner = Perms.Node.get_owner nperms in
-		Quota.check store.quota new_owner 0;
+		if old_owner <> new_owner then Quota.check store.quota new_owner 0;
 		store.root <- path_setperms store perm path nperms;
 		Quota.del_entry store.quota old_owner;
 		Quota.add_entry store.quota new_owner

On Thu, 2013-10-31 at 16:32 +0000, Thomas Sanders wrote:
> Here I''m submitting to xen-devel a patch written by Zheng Li.
If you "git commit --author=''Zheng Li
<...@citrix.com>''" and then use
git send-email it will do the right thing which is $subject "CA-108294:
allow a domain ..." and then a pseudoheader in the body:
  From: Zheng Li <...@citrix.com>

  <commit message>

  S-o-b: ...

The git tools then consume this on my end and DTRT. It also removes the
need for you to write a little preamble explaining who wrote the patch.

BTW, a shorter subject, omitting the CA-xxx and including a oxenstored:
prefix would be appreciated too.

I can do all this as I commit but it seems like you might be sending
more such patches in the future so thought I''d bring it up ;-)
> Thomas Sanders
> 
> 
> 
> CA-108294: allow a domain updating existing xenstore keys even if it
> has already reached its max entries limit
>
> As updating existing key won''t increase the number of entries
> belonging to a domain, we should avoid checking the max entries limit
> prematurely. The patch addresses this issue in the following
> functions: write/add, mkdir, setperms. 
> 
> Signed-off-by: Zheng Li <zheng.li@eu.citrix.com>
The patch looks good to me, in so far as I speak ocaml so:
Acked-by: Ian Campbell <ian.campbell@citrix.com>

But I''d much prefer to have Dave''s ACK too.

I think it would be appropriate (but not mandatory) for you to either
Signed-off-by, Acked-by or Reviewed-by the patch yourself since you have
forwarded it on. For forms sake and per the DCO an S-o-b should probably
be added.
> +	(* It''s upt to the mkdir logic to decide what to do with
existing path *)
typo. I can fix on commit, assuming I remember.

Ian.

On 01 November 2013 at 4:19 PM, Ian Campbell wrote:
> If you [issue certain git commands]
> The git tools then consume this on my end and DTRT. It also removes the
> need for you to write a little preamble explaining who wrote the patch.
> 
> BTW, a shorter subject, omitting the CA-xxx and including a oxenstored:
> prefix would be appreciated too.
> 
> I can do all this as I commit but it seems like you might be sending
> more such patches in the future so thought I''d bring it up ;-)
Thank you. That''s all useful. I''ll bear it in mind in future.
> The patch looks good to me, in so far as I speak ocaml so:
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> But I''d much prefer to have Dave''s ACK too.
Makes sense. This morning he told me, "I''ll reply in a bit."
> I think it would be appropriate (but not mandatory) for you to either
> Signed-off-by, Acked-by or Reviewed-by the patch yourself since you have
> forwarded it on. For forms sake and per the DCO an S-o-b should probably
> be added.
I''ve reviewed it.

It''s signed-off-by Zheng; he''s the sole author so I
didn''t think S-o-b
from me would make sense.

I''m adding Zheng to this email''s CC list though.
> > +	(* It''s upt to the mkdir logic to decide what to do with
existing path *)
> 
> typo. I can fix on commit, assuming I remember.
> 
> Ian.

On Fri, 2013-11-01 at 16:40 +0000, Thomas Sanders wrote:
> > I think it would be appropriate (but not mandatory) for you to either
> > Signed-off-by, Acked-by or Reviewed-by the patch yourself since you
have
> > forwarded it on. For forms sake and per the DCO an S-o-b should
probably
> > be added.
> 
> I''ve reviewed it.
> 
> It''s signed-off-by Zheng; he''s the sole author so I
didn''t think S-o-b
> from me would make sense.
It would come under 3c of the DCO I think, see
http://wiki.xen.org/wiki/Submitting_Xen_Patches#Signing_off_a_patch for
the text of what adding your S-o-b means.

It''s not so important when the forwarder and the originator work for
the
same organisation who owns the copyright anyway (IMHO, IANAL, etc).

Ian.

On Fri, 2013-11-01 at 16:40 +0000, Thomas Sanders wrote:
> On 01 November 2013 at 4:19 PM, Ian Campbell wrote:
> > If you [issue certain git commands]
> > The git tools then consume this on my end and DTRT. It also removes
the
> > need for you to write a little preamble explaining who wrote the
patch.
> > 
> > BTW, a shorter subject, omitting the CA-xxx and including a
oxenstored:
> > prefix would be appreciated too.
> > 
> > I can do all this as I commit but it seems like you might be sending
> > more such patches in the future so thought I''d bring it up
;-)
> 
> Thank you. That''s all useful. I''ll bear it in mind in
future.
> 
> > The patch looks good to me, in so far as I speak ocaml so:
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> > But I''d much prefer to have Dave''s ACK too.
> 
> Makes sense. This morning he told me, "I''ll reply in a
bit."
Or maybe two bits ;-)

On Tue, 2013-11-05 at 11:46 +0000, Ian Campbell wrote:
> On Fri, 2013-11-01 at 16:40 +0000, Thomas Sanders wrote:
> > On 01 November 2013 at 4:19 PM, Ian Campbell wrote:
> > > If you [issue certain git commands]
> > > The git tools then consume this on my end and DTRT. It also
removes the
> > > need for you to write a little preamble explaining who wrote the
patch.
> > > 
> > > BTW, a shorter subject, omitting the CA-xxx and including a
oxenstored:
> > > prefix would be appreciated too.
> > > 
> > > I can do all this as I commit but it seems like you might be
sending
> > > more such patches in the future so thought I''d bring it
up ;-)
> > 
> > Thank you. That''s all useful. I''ll bear it in mind
in future.
> > 
> > > The patch looks good to me, in so far as I speak ocaml so:
> > > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > > 
> > > But I''d much prefer to have Dave''s ACK too.
> > 
> > Makes sense. This morning he told me, "I''ll reply in a
bit."
> 
> Or maybe two bits ;-)
I got bored of waiting. Applied.

Ian.