From c0595d17ff8df87af95cbd3f4a430eecaf85cbf1 Mon Sep 17 00:00:00 2001
From: Tatsuhiro Tsujikawa <tatsuhiro.t@gmail.com>
Date: Sun, 4 Oct 2009 09:01:11 +0000
Subject: [PATCH] 2009-10-04  Tatsuhiro Tsujikawa 
 <t-tujikawa@users.sourceforge.net>

	Fixed the bug that the option values changed by XML-RPC
	method(changeOption and changeGlobalOption) are overwritten to the
	previous value by the next these request which doesn't contain
	that option value. Supporse max-download-limit is initially 0. You
	changed this value to 100K by changeOption. Then you issue
	changeOption request to change max-upload-limit to 50K. This
	second request doesn't contain xml-download-limit, so it is back
	to initial value, 0. Another improvement is that exception is
	thrown when changeOption and changeGlobalOption request contains
	option name which doesn't allowed in each request.
	* src/DownloadEngine.h
	* src/XmlRpcMethod.cc
	* src/XmlRpcMethod.h
	* src/XmlRpcMethodImpl.cc
	* src/download_helper.cc
	* src/download_helper.h
	* test/XmlRpcMethodTest.cc
---
 ChangeLog                | 20 +++++++++++
 src/DownloadEngine.h     |  2 +-
 src/XmlRpcMethod.cc      | 76 ++++++++++++++++++++++++++++++----------
 src/XmlRpcMethod.h       |  8 +++++
 src/XmlRpcMethodImpl.cc  | 20 ++++++-----
 src/download_helper.cc   |  6 ++--
 src/download_helper.h    |  4 +--
 test/XmlRpcMethodTest.cc | 38 ++++++++++++++++++++
 8 files changed, 142 insertions(+), 32 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index be6b2780..20da39a1 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,23 @@
+2009-10-04  Tatsuhiro Tsujikawa  <t-tujikawa@users.sourceforge.net>
+
+	Fixed the bug that the option values changed by XML-RPC
+	method(changeOption and changeGlobalOption) are overwritten to the
+	previous value by the next these request which doesn't contain
+	that option value. Supporse max-download-limit is initially 0. You
+	changed this value to 100K by changeOption. Then you issue
+	changeOption request to change max-upload-limit to 50K. This
+	second request doesn't contain xml-download-limit, so it is back
+	to initial value, 0. Another improvement is that exception is
+	thrown when changeOption and changeGlobalOption request contains
+	option name which doesn't allowed in each request.
+	* src/DownloadEngine.h
+	* src/XmlRpcMethod.cc
+	* src/XmlRpcMethod.h
+	* src/XmlRpcMethodImpl.cc
+	* src/download_helper.cc
+	* src/download_helper.h
+	* test/XmlRpcMethodTest.cc
+
 2009-10-04  Tatsuhiro Tsujikawa  <t-tujikawa@users.sourceforge.net>
 
 	Added missing MetalinkParserStateImpl.{cc,h}
diff --git a/src/DownloadEngine.h b/src/DownloadEngine.h
index 13568c28..401142d7 100644
--- a/src/DownloadEngine.h
+++ b/src/DownloadEngine.h
@@ -155,7 +155,7 @@ public:
   SharedHandle<RequestGroupMan> _requestGroupMan;
   SharedHandle<FileAllocationMan> _fileAllocationMan;
   SharedHandle<CheckIntegrityMan> _checkIntegrityMan;
-  const Option* option;
+  Option* option;
   
   DownloadEngine(const SharedHandle<EventPoll>& eventPoll);
 
diff --git a/src/XmlRpcMethod.cc b/src/XmlRpcMethod.cc
index 29a320a6..0253d9fd 100644
--- a/src/XmlRpcMethod.cc
+++ b/src/XmlRpcMethod.cc
@@ -46,6 +46,7 @@
 #include "XmlRpcRequest.h"
 #include "XmlRpcResponse.h"
 #include "prefs.h"
+#include "StringFormat.h"
 
 namespace aria2 {
 
@@ -74,23 +75,32 @@ XmlRpcResponse XmlRpcMethod::execute
   }
 }
 
-
 template<typename InputIterator>
 static void gatherOption
 (InputIterator first, InputIterator last,
- const SharedHandle<Option>& option, const BDE& optionsDict,
+ const std::set<std::string>& changeableOptions,
+ const SharedHandle<Option>& option,
  const SharedHandle<OptionParser>& optionParser)
 {
   for(; first != last; ++first) {
-    if(optionsDict.containsKey(*first)) {
-      const BDE& value = optionsDict[*first];
+    const std::string& optionName = (*first).first;
+    if(changeableOptions.find(optionName) == changeableOptions.end()) {
+      throw DL_ABORT_EX
+	(StringFormat
+	 ("%s cannot be changed or unknown option.", optionName.c_str()).str());
+    } else {
       SharedHandle<OptionHandler> optionHandler =
-	optionParser->findByName(*first);
+	optionParser->findByName(optionName);
       if(optionHandler.isNull()) {
-	continue;
+	throw DL_ABORT_EX
+	  (StringFormat
+	   ("We don't know how to deal with %s option",
+	    optionName.c_str()).str());
       }
       // header and index-out option can take array as value
-      if((*first == PREF_HEADER || *first == PREF_INDEX_OUT) && value.isList()){
+      const BDE& value = (*first).second;
+      if((optionName == PREF_HEADER || optionName == PREF_INDEX_OUT) &&
+	 value.isList()){
 	for(BDE::List::const_iterator argiter = value.listBegin();
 	    argiter != value.listEnd(); ++argiter) {
 	  if((*argiter).isString()) {
@@ -107,17 +117,33 @@ static void gatherOption
 void XmlRpcMethod::gatherRequestOption
 (const SharedHandle<Option>& option, const BDE& optionsDict)
 {
-  gatherOption(listRequestOptions().begin(), listRequestOptions().end(),
-	       option, optionsDict, _optionParser);
+  gatherOption(optionsDict.dictBegin(), optionsDict.dictEnd(),
+	       listRequestOptions(),
+	       option, _optionParser);
 }
 
-const std::vector<std::string>& listChangeableOptions()
+// Copy option in the range [optNameFirst, optNameLast) from src to
+// dest.
+template<typename InputIterator>
+static void applyOption(InputIterator optNameFirst,
+			InputIterator optNameLast,
+			Option* dest,
+			Option* src)
+{
+  for(; optNameFirst != optNameLast; ++optNameFirst) {
+    if(src->defined(*optNameFirst)) {
+      dest->put(*optNameFirst, src->get(*optNameFirst));
+    }
+  }
+}
+
+const std::set<std::string>& listChangeableOptions()
 {
   static const std::string OPTIONS[] = {
     PREF_MAX_UPLOAD_LIMIT,
     PREF_MAX_DOWNLOAD_LIMIT,
   };
-  static std::vector<std::string> options
+  static std::set<std::string> options
     (&OPTIONS[0], &OPTIONS[arrayLength(OPTIONS)]);
   return options;
 }
@@ -125,18 +151,25 @@ const std::vector<std::string>& listChangeableOptions()
 void XmlRpcMethod::gatherChangeableOption
 (const SharedHandle<Option>& option, const BDE& optionsDict)
 {
-  gatherOption(listChangeableOptions().begin(), listChangeableOptions().end(),
-	       option, optionsDict, _optionParser);
+  gatherOption(optionsDict.dictBegin(), optionsDict.dictEnd(),
+	       listChangeableOptions(),
+	       option, _optionParser);
 }
 
-const std::vector<std::string>& listChangeableGlobalOptions()
+void XmlRpcMethod::applyChangeableOption(Option* dest, Option* src) const
+{
+  applyOption(listChangeableOptions().begin(), listChangeableOptions().end(),
+	      dest, src);
+}
+
+const std::set<std::string>& listChangeableGlobalOptions()
 {
   static const std::string OPTIONS[] = {
     PREF_MAX_OVERALL_UPLOAD_LIMIT,
     PREF_MAX_OVERALL_DOWNLOAD_LIMIT,
     PREF_MAX_CONCURRENT_DOWNLOADS,
   };
-  static std::vector<std::string> options
+  static std::set<std::string> options
     (&OPTIONS[0], &OPTIONS[arrayLength(OPTIONS)]);
   return options;
 }
@@ -144,9 +177,16 @@ const std::vector<std::string>& listChangeableGlobalOptions()
 void XmlRpcMethod::gatherChangeableGlobalOption
 (const SharedHandle<Option>& option, const BDE& optionsDict)
 {
-  gatherOption(listChangeableGlobalOptions().begin(),
-	       listChangeableGlobalOptions().end(),
-	       option, optionsDict, _optionParser);
+  gatherOption(optionsDict.dictBegin(), optionsDict.dictEnd(),
+	       listChangeableGlobalOptions(),
+	       option, _optionParser);
+}
+
+void XmlRpcMethod::applyChangeableGlobalOption(Option* dest, Option* src) const
+{
+  applyOption(listChangeableGlobalOptions().begin(),
+	      listChangeableGlobalOptions().end(),
+	      dest, src);
 }
 
 } // namespace xmlrpc
diff --git a/src/XmlRpcMethod.h b/src/XmlRpcMethod.h
index c9ed0654..476917f2 100644
--- a/src/XmlRpcMethod.h
+++ b/src/XmlRpcMethod.h
@@ -78,8 +78,16 @@ protected:
   void gatherChangeableOption(const SharedHandle<Option>& option,
 			      const BDE& optionDict);
 
+  // Copy options which is changeable in XML-RPC changeOption command
+  // to dest.
+  void applyChangeableOption(Option* dest, Option* src) const;
+
   void gatherChangeableGlobalOption(const SharedHandle<Option>& option,
 				    const BDE& optionDict);
+
+  // Copy options which is changeable in XML-RPC changeGlobalOption
+  // command to dest.
+  void applyChangeableGlobalOption(Option* dest, Option* src) const;
 public:
   XmlRpcMethod();
 
diff --git a/src/XmlRpcMethodImpl.cc b/src/XmlRpcMethodImpl.cc
index c08016bd..0eafa1fd 100644
--- a/src/XmlRpcMethodImpl.cc
+++ b/src/XmlRpcMethodImpl.cc
@@ -592,15 +592,17 @@ BDE ChangeOptionXmlRpcMethod::process
     throw DL_ABORT_EX
       (StringFormat("Cannot change option for GID#%d", gid).str());
   }
-  SharedHandle<Option> option(new Option(*group->getOption().get()));
+  SharedHandle<Option> option(new Option());
   if(params.size() > 1 && params[1].isDict()) {
     gatherChangeableOption(option, params[1]);
-  }
-  if(option->defined(PREF_MAX_DOWNLOAD_LIMIT)) {
-    group->setMaxDownloadSpeedLimit(option->getAsInt(PREF_MAX_DOWNLOAD_LIMIT));
-  }
-  if(option->defined(PREF_MAX_UPLOAD_LIMIT)) {
-    group->setMaxUploadSpeedLimit(option->getAsInt(PREF_MAX_UPLOAD_LIMIT));
+    applyChangeableOption(group->getOption().get(), option.get());
+    if(option->defined(PREF_MAX_DOWNLOAD_LIMIT)) {
+      group->setMaxDownloadSpeedLimit
+	(option->getAsInt(PREF_MAX_DOWNLOAD_LIMIT));
+    }
+    if(option->defined(PREF_MAX_UPLOAD_LIMIT)) {
+      group->setMaxUploadSpeedLimit(option->getAsInt(PREF_MAX_UPLOAD_LIMIT));
+    }
   }
   return BDE_OK;
 }
@@ -613,8 +615,10 @@ BDE ChangeGlobalOptionXmlRpcMethod::process
   if(params.empty() || !params[0].isDict()) {
     return BDE_OK;
   }
-  SharedHandle<Option> option(new Option(*e->option));
+  SharedHandle<Option> option(new Option());
   gatherChangeableGlobalOption(option, params[0]);
+  applyChangeableGlobalOption(e->option, option.get());
+
   if(option->defined(PREF_MAX_OVERALL_DOWNLOAD_LIMIT)) {
     e->_requestGroupMan->setMaxOverallDownloadSpeedLimit
       (option->getAsInt(PREF_MAX_OVERALL_DOWNLOAD_LIMIT));
diff --git a/src/download_helper.cc b/src/download_helper.cc
index 2a4bfa8d..4a26312c 100644
--- a/src/download_helper.cc
+++ b/src/download_helper.cc
@@ -67,7 +67,7 @@
 
 namespace aria2 {
 
-const std::vector<std::string>& listRequestOptions()
+const std::set<std::string>& listRequestOptions()
 {
   static const std::string REQUEST_OPTIONS[] = {
     PREF_DIR,
@@ -138,7 +138,7 @@ const std::vector<std::string>& listRequestOptions()
     PREF_PARAMETERIZED_URI,
     PREF_REALTIME_CHUNK_CHECKSUM
   };
-  static std::vector<std::string> requestOptions
+  static std::set<std::string> requestOptions
     (&REQUEST_OPTIONS[0],
      &REQUEST_OPTIONS[arrayLength(REQUEST_OPTIONS)]);;
 
@@ -385,7 +385,7 @@ static void createRequestGroupForUriList
     }
 
     SharedHandle<Option> requestOption(new Option(*option.get()));
-    for(std::vector<std::string>::const_iterator i =
+    for(std::set<std::string>::const_iterator i =
 	  listRequestOptions().begin(); i != listRequestOptions().end(); ++i) {
       if(tempOption->defined(*i)) {
 	requestOption->put(*i, tempOption->get(*i));
diff --git a/src/download_helper.h b/src/download_helper.h
index 44fe3bc2..41d7d914 100644
--- a/src/download_helper.h
+++ b/src/download_helper.h
@@ -39,7 +39,7 @@
 
 #include <string>
 #include <deque>
-#include <vector>
+#include <set>
 
 #include "SharedHandle.h"
 
@@ -48,7 +48,7 @@ namespace aria2 {
 class RequestGroup;
 class Option;
 
-const std::vector<std::string>& listRequestOptions();
+const std::set<std::string>& listRequestOptions();
 
 #ifdef ENABLE_BITTORRENT
 // Create RequestGroup object using torrent file specified by
diff --git a/test/XmlRpcMethodTest.cc b/test/XmlRpcMethodTest.cc
index 87bd8e5a..39461a2e 100644
--- a/test/XmlRpcMethodTest.cc
+++ b/test/XmlRpcMethodTest.cc
@@ -46,9 +46,11 @@ class XmlRpcMethodTest:public CppUnit::TestFixture {
 #endif // ENABLE_METALINK
   CPPUNIT_TEST(testChangeOption);
   CPPUNIT_TEST(testChangeOption_withBadOption);
+  CPPUNIT_TEST(testChangeOption_withNotAllowedOption);
   CPPUNIT_TEST(testChangeOption_withoutGid);
   CPPUNIT_TEST(testChangeGlobalOption);
   CPPUNIT_TEST(testChangeGlobalOption_withBadOption);
+  CPPUNIT_TEST(testChangeGlobalOption_withNotAllowedOption);
   CPPUNIT_TEST(testTellStatus_withoutGid);
   CPPUNIT_TEST(testTellWaiting);
   CPPUNIT_TEST(testTellWaiting_fail);
@@ -94,9 +96,11 @@ public:
 #endif // ENABLE_METALINK
   void testChangeOption();
   void testChangeOption_withBadOption();
+  void testChangeOption_withNotAllowedOption();
   void testChangeOption_withoutGid();
   void testChangeGlobalOption();
   void testChangeGlobalOption_withBadOption();
+  void testChangeGlobalOption_withNotAllowedOption();
   void testTellStatus_withoutGid();
   void testTellWaiting();
   void testTellWaiting_fail();
@@ -369,8 +373,12 @@ void XmlRpcMethodTest::testChangeOption()
   CPPUNIT_ASSERT_EQUAL(0, res._code);
   CPPUNIT_ASSERT_EQUAL((unsigned int)100*1024,
 		       group->getMaxDownloadSpeedLimit());
+  CPPUNIT_ASSERT_EQUAL(std::string("102400"),
+		       group->getOption()->get(PREF_MAX_DOWNLOAD_LIMIT));
 #ifdef ENABLE_BITTORRENT
    CPPUNIT_ASSERT_EQUAL((unsigned int)50*1024, group->getMaxUploadSpeedLimit());
+   CPPUNIT_ASSERT_EQUAL(std::string("51200"),
+			group->getOption()->get(PREF_MAX_UPLOAD_LIMIT));
 #endif // ENABLE_BITTORRENT
 }
 
@@ -389,6 +397,21 @@ void XmlRpcMethodTest::testChangeOption_withBadOption()
   CPPUNIT_ASSERT_EQUAL(1, res._code);
 }
 
+void XmlRpcMethodTest::testChangeOption_withNotAllowedOption()
+{
+  SharedHandle<RequestGroup> group(new RequestGroup(_option));
+  _e->_requestGroupMan->addReservedGroup(group);
+
+  ChangeOptionXmlRpcMethod m;
+  XmlRpcRequest req("aria2.changeOption", BDE::list());
+  req._params << BDE("1");
+  BDE opt = BDE::dict();
+  opt[PREF_MAX_OVERALL_DOWNLOAD_LIMIT] = BDE("100K");
+  req._params << opt;
+  XmlRpcResponse res = m.execute(req, _e.get());
+  CPPUNIT_ASSERT_EQUAL(1, res._code);
+}
+
 void XmlRpcMethodTest::testChangeOption_withoutGid()
 {
   ChangeOptionXmlRpcMethod m;
@@ -412,9 +435,13 @@ void XmlRpcMethodTest::testChangeGlobalOption()
   CPPUNIT_ASSERT_EQUAL(0, res._code);
   CPPUNIT_ASSERT_EQUAL((unsigned int)100*1024,
 		       _e->_requestGroupMan->getMaxOverallDownloadSpeedLimit());
+  CPPUNIT_ASSERT_EQUAL(std::string("102400"),
+		       _e->option->get(PREF_MAX_OVERALL_DOWNLOAD_LIMIT));
 #ifdef ENABLE_BITTORRENT
   CPPUNIT_ASSERT_EQUAL((unsigned int)50*1024,
 		       _e->_requestGroupMan->getMaxOverallUploadSpeedLimit());
+  CPPUNIT_ASSERT_EQUAL(std::string("51200"),
+		       _e->option->get(PREF_MAX_OVERALL_UPLOAD_LIMIT));
 #endif // ENABLE_BITTORRENT
 }
 
@@ -429,6 +456,17 @@ void XmlRpcMethodTest::testChangeGlobalOption_withBadOption()
   CPPUNIT_ASSERT_EQUAL(1, res._code);
 }
 
+void XmlRpcMethodTest::testChangeGlobalOption_withNotAllowedOption()
+{
+  ChangeGlobalOptionXmlRpcMethod m;
+  XmlRpcRequest req("aria2.changeGlobalOption", BDE::list());
+  BDE opt = BDE::dict();
+  opt[PREF_MAX_DOWNLOAD_LIMIT] = BDE("100K");
+  req._params << opt;
+  XmlRpcResponse res = m.execute(req, _e.get());
+  CPPUNIT_ASSERT_EQUAL(1, res._code);
+}
+
 void XmlRpcMethodTest::testNoSuchMethod()
 {
   NoSuchMethodXmlRpcMethod m;