From 50109183de7d43ab62b16bd1f770678afe3bac60 Mon Sep 17 00:00:00 2001 From: Ricardo Abreu Date: Fri, 22 Feb 2019 19:30:44 +0100 Subject: [PATCH] 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 | 7 ++-- src/daemon/daemon.cpp | 67 +++++++++++++++++++++------------------ src/utils/utils.cpp | 34 +++++++++++++++++--- tests/test_utils.cpp | 16 +++++----- 4 files changed, 78 insertions(+), 46 deletions(-) diff --git a/include/multipass/utils.h b/include/multipass/utils.h index ea1bf80805..5f1c2605b7 100644 --- a/include/multipass/utils.h +++ b/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 +#include #include #include @@ -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 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& args, QuoteType type); bool run_cmd_for_status(const QString& cmd, const QStringList& args, const int timeout=30000); diff --git a/src/daemon/daemon.cpp b/src/daemon/daemon.cpp index 52f8e269e8..c818bdd02a 100644 --- a/src/daemon/daemon.cpp +++ b/src/daemon/daemon.cpp @@ -52,6 +52,7 @@ #include #include #include +#include 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 -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); diff --git a/src/utils/utils.cpp b/src/utils/utils.cpp index 2a0b2ee60b..d7286e3ec8 100644 --- a/src/utils/utils.cpp +++ b/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 #include +#include #include #include #include @@ -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 { + 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) diff --git a/tests/test_utils.cpp b/tests/test_utils.cpp index c2a0a01681..1ba6d100d8 100644 --- a/tests/test_utils.cpp +++ b/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)