Implement memory value normalization.

Implement verification and normalization of memory values in a single step. Use that in the daemon, replacing mere validation. Update hostname validation for consistency. Update tests accordingly. Fixes #616.

include/multipass/utils.h

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2017-2018 Canonical, Ltd.
+ * Copyright (C) 2017-2019 Canonical, Ltd.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -19,6 +19,7 @@
 #define MULTIPASS_UTILS_H
 
 #include <multipass/path.h>
+#include <multipass/optional.h>
 #include <multipass/virtual_machine.h>
 
 #include <chrono>
@@ -51,8 +52,8 @@ QString make_uuid();
 std::string contents_of(const multipass::Path& file_path);
 bool has_only_digits(const std::string& value);
 void validate_server_address(const std::string& value);
-bool valid_memory_value(const QString& mem_string);
-bool valid_hostname(const QString& name_string);
+optional<std::string> normalize_memory_value(const std::string& mem_value);
+bool valid_hostname(const std::string& name_string);
 bool invalid_target_path(const QString& target_path);
 std::string to_cmd(const std::vector<std::string>& args, QuoteType type);
 bool run_cmd_for_status(const QString& cmd, const QStringList& args, const int timeout=30000);

src/daemon/daemon.cpp

@@ -52,6 +52,7 @@
 #include <functional>
 #include <stdexcept>
 #include <unordered_set>
+#include <utility>
 
 namespace mp = multipass;
 namespace mpl = multipass::logging;
@@ -154,24 +155,22 @@ void prepare_user_data(YAML::Node& user_data_config, YAML::Node& vendor_config)
 }
 
 mp::VirtualMachineDescription to_machine_desc(const mp::LaunchRequest* request, const std::string& name,
+                                              const std::string& mem_size, const std::string& disk_space,
                                               const std::string& mac_addr, const std::string& ssh_username,
                                               const mp::VMImage& image, YAML::Node& meta_data_config,
                                               YAML::Node& user_data_config, YAML::Node& vendor_data_config,
                                               const mp::SSHKeyProvider& key_provider)
 {
     const auto num_cores = request->num_cores() < 1 ? 1 : request->num_cores();
-    const auto mem_size = request->mem_size().empty() ? "1G" : request->mem_size();
-    const auto disk_size = request->disk_space().empty() ? "5G" : request->disk_space();
     const auto instance_dir = mp::utils::base_dir(image.image_path);
     const auto cloud_init_iso =
         make_cloud_init_image(name, instance_dir, meta_data_config, user_data_config, vendor_data_config);
-    return {num_cores, mem_size, disk_size, name, mac_addr, ssh_username, image, cloud_init_iso, key_provider};
+    return {num_cores, mem_size, disk_space, name, mac_addr, ssh_username, image, cloud_init_iso, key_provider};
 }
 
 template <typename T>
-auto name_from(const mp::LaunchRequest* request, mp::NameGenerator& name_gen, const T& currently_used_names)
+auto name_from(const std::string& requested_name, mp::NameGenerator& name_gen, const T& currently_used_names)
 {
-    auto requested_name = request->instance_name();
     if (requested_name.empty())
     {
         auto name = name_gen.make_name();
@@ -281,25 +280,29 @@ auto fetch_image_for(const std::string& name, const mp::FetchType& fetch_type, m
 
 auto validate_create_arguments(const mp::LaunchRequest* request)
 {
-    mp::LaunchError launch_error;
+    auto mem_size = request->mem_size();
+    auto disk_space = request->disk_space();
+    auto instance_name = request->instance_name();
+    auto option_errors = mp::LaunchError{};
 
-    if (!request->disk_space().empty() && !mp::utils::valid_memory_value(QString::fromStdString(request->disk_space())))
-    {
-        launch_error.add_error_codes(mp::LaunchError::INVALID_DISK_SIZE);
-    }
+    const auto opt_mem_size = mp::utils::normalize_memory_value(mem_size.empty() ? "1G" : mem_size);
+    const auto opt_disk_space = mp::utils::normalize_memory_value(disk_space.empty() ? "5G" : disk_space);
 
-    if (!request->mem_size().empty() && !mp::utils::valid_memory_value(QString::fromStdString(request->mem_size())))
-    {
-        launch_error.add_error_codes(mp::LaunchError::INVALID_MEM_SIZE);
-    }
+    if(opt_mem_size)
+        mem_size = *opt_mem_size;
+    else
+        option_errors.add_error_codes(mp::LaunchError::INVALID_MEM_SIZE);
 
-    if (!request->instance_name().empty() &&
-        !mp::utils::valid_hostname(QString::fromStdString(request->instance_name())))
-    {
-        launch_error.add_error_codes(mp::LaunchError::INVALID_HOSTNAME);
-    }
+    if(opt_disk_space)
+        disk_space = *opt_disk_space;
+    else
+        option_errors.add_error_codes(mp::LaunchError::INVALID_DISK_SIZE);
+
+    if(!request->instance_name().empty() &&
+       !mp::utils::valid_hostname(request->instance_name()))
+        option_errors.add_error_codes(mp::LaunchError::INVALID_HOSTNAME);
 
-    return launch_error;
+    return make_tuple(mem_size, disk_space, instance_name, option_errors);
 }
 
 auto grpc_status_for_mount_error(const std::string& instance_name)
@@ -666,7 +669,19 @@ try // clang-format on
     if (metrics_opt_in.opt_in_status == OptInStatus::ACCEPTED)
         metrics_provider.send_metrics();
 
-    auto name = name_from(request, *config->name_generator, vm_instances);
+    auto mem_disk_name_and_errors = validate_create_arguments(request);
+    auto mem_size = std::get<0>(mem_disk_name_and_errors); // use structured bindings instead in C++17
+    auto disk_space = std::get<1>(mem_disk_name_and_errors); // use structured bindings instead in C++17
+    auto instance_name = std::get<2>(mem_disk_name_and_errors); // use structured bindings instead in C++17
+    auto option_errors = std::get<3>(mem_disk_name_and_errors); // use structured bindings instead in C++17
+
+    if (!option_errors.error_codes().empty())
+    {
+        return grpc::Status(grpc::StatusCode::INVALID_ARGUMENT, "Invalid arguments supplied",
+                            option_errors.SerializeAsString());
+    }
+
+    auto name = name_from(instance_name, *config->name_generator, vm_instances);
 
     if (vm_instances.find(name) != vm_instances.end() || deleted_instances.find(name) != deleted_instances.end())
     {
@@ -681,14 +696,6 @@ try // clang-format on
 
     config->factory->check_hypervisor_support();
 
-    auto option_errors = validate_create_arguments(request);
-
-    if (!option_errors.error_codes().empty())
-    {
-        return grpc::Status(grpc::StatusCode::INVALID_ARGUMENT, "Invalid arguments supplied",
-                            option_errors.SerializeAsString());
-    }
-
     auto progress_monitor = [server](int progress_type, int percentage) {
         LaunchReply create_reply;
         create_reply.mutable_launch_progress()->set_percent_complete(std::to_string(percentage));
@@ -733,7 +740,7 @@ try // clang-format on
         }
     }
     auto vm_desc =
-        to_machine_desc(request, name, mac_addr, config->ssh_username, vm_image, meta_data_cloud_init_config,
+        to_machine_desc(request, name, mem_size, disk_space, mac_addr, config->ssh_username, vm_image, meta_data_cloud_init_config,
                         user_data_cloud_init_config, vendor_data_cloud_init_config, *config->ssh_key_provider);
 
     config->factory->prepare_instance_image(vm_image, vm_desc);

src/utils/utils.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2017-2018 Canonical, Ltd.
+ * Copyright (C) 2017-2019 Canonical, Ltd.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -29,6 +29,7 @@
 
 #include <algorithm>
 #include <array>
+#include <cassert>
 #include <cctype>
 #include <fstream>
 #include <random>
@@ -54,18 +55,41 @@ QDir mp::utils::base_dir(const QString& path)
     return info.absoluteDir();
 }
 
-bool mp::utils::valid_memory_value(const QString& mem_string)
+auto mp::utils::normalize_memory_value(const std::string& mem_value) -> optional<std::string>
 {
+    static constexpr auto kilo = 1024LL;
+
     QRegExp matcher("^(\\d+)([KMG])?B?$", Qt::CaseInsensitive);
 
-    return matcher.exactMatch(mem_string);
+    if(matcher.exactMatch(QString::fromStdString(mem_value)))
+    {
+        auto val = std::stoll(matcher.cap(1).toStdString()); // value is in the second capture (1st one is the whole match)
+        const auto unit = matcher.cap(2); // unit in the third capture (idem)
+        if(!unit.isEmpty())
+        {
+            switch(unit.front().toLower().toLatin1())
+            {
+            case 'g': val *= kilo;
+                [[fallthrough]]; // absent break on purpose
+            case 'm': val *= kilo;
+                [[fallthrough]]; // absent break on purpose
+            case 'k': val *= kilo;
+                break;
+            default: assert(false && "Shouldn't be here (invalid unit)");
+            }
+        }
+
+        return std::to_string(val) + "B";
+    }
+
+    return {};
 }
 
-bool mp::utils::valid_hostname(const QString& name_string)
+bool mp::utils::valid_hostname(const std::string& name_string)
 {
     QRegExp matcher("^([a-zA-Z]|[a-zA-Z][a-zA-Z0-9\\-]*[a-zA-Z0-9])");
 
-    return matcher.exactMatch(name_string);
+    return matcher.exactMatch(QString::fromStdString(name_string));
 }
 
 bool mp::utils::invalid_target_path(const QString& target_path)

tests/test_utils.cpp

@@ -176,42 +176,42 @@ TEST(Utils, decimal_is_invalid_memory_value)
 
 TEST(Utils, hostname_begins_with_letter_is_valid)
 {
-    EXPECT_TRUE(mp::utils::valid_hostname(QString("foo")));
+    EXPECT_TRUE(mp::utils::valid_hostname("foo"));
 }
 
 TEST(Utils, hostname_single_letter_is_valid)
 {
-    EXPECT_TRUE(mp::utils::valid_hostname(QString("f")));
+    EXPECT_TRUE(mp::utils::valid_hostname("f"));
 }
 
 TEST(Utils, hostname_contains_digit_is_valid)
 {
-    EXPECT_TRUE(mp::utils::valid_hostname(QString("foo1")));
+    EXPECT_TRUE(mp::utils::valid_hostname("foo1"));
 }
 
 TEST(Utils, hostname_contains_hyphen_is_valid)
 {
-    EXPECT_TRUE(mp::utils::valid_hostname(QString("foo-bar")));
+    EXPECT_TRUE(mp::utils::valid_hostname("foo-bar"));
 }
 
 TEST(Utils, hostname_begins_with_digit_is_invalid)
 {
-    EXPECT_FALSE(mp::utils::valid_hostname(QString("1foo")));
+    EXPECT_FALSE(mp::utils::valid_hostname("1foo"));
 }
 
 TEST(Utils, hostname_single_digit_is_invalid)
 {
-    EXPECT_FALSE(mp::utils::valid_hostname(QString("1")));
+    EXPECT_FALSE(mp::utils::valid_hostname("1"));
 }
 
 TEST(Utils, hostname_contains_underscore_is_invalid)
 {
-    EXPECT_FALSE(mp::utils::valid_hostname(QString("foo_bar")));
+    EXPECT_FALSE(mp::utils::valid_hostname("foo_bar"));
 }
 
 TEST(Utils, hostname_contains_special_character_is_invalid)
 {
-    EXPECT_FALSE(mp::utils::valid_hostname(QString("foo!")));
+    EXPECT_FALSE(mp::utils::valid_hostname("foo!"));
 }
 
 TEST(Utils, path_root_invalid)