Skip to content

Commit

Permalink
crostini: Volumes record the VM type
Browse files Browse the repository at this point in the history
Bug: b:230667118
Test: Unit tests

Change-Id: Ie1a7ee3cd1a69fe624ea8087f96b72bf1b8f1b91
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3626655
Reviewed-by: Fergus Dall <sidereal@google.com>
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Commit-Queue: David Munro <davidmunro@google.com>
Cr-Commit-Position: refs/heads/main@{#1002415}
  • Loading branch information
David Munro authored and Chromium LUCI CQ committed May 12, 2022
1 parent be8c633 commit 5e249db
Show file tree
Hide file tree
Showing 11 changed files with 77 additions and 19 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@
#include "chrome/browser/ash/file_manager/volume_manager.h"
#include "chrome/browser/ash/guest_os/public/guest_os_mount_provider.h"
#include "chrome/browser/ash/guest_os/public/guest_os_service.h"
#include "chrome/browser/ash/guest_os/public/types.h"
#include "chrome/browser/ash/smb_client/smb_service.h"
#include "chrome/browser/ash/smb_client/smb_service_factory.h"
#include "chrome/browser/browser_process.h"
Expand Down Expand Up @@ -1704,13 +1705,16 @@ class MockGuestOsMountProvider : public guest_os::GuestOsMountProvider {
return crostini::ContainerId::GetDefault();
}

guest_os::VmType vm_type() override {
return guest_os::VmType::ApplicationList_VmType_TERMINA;
}

int cid_;
int cid() override { return cid_; }

private:
Profile* profile_;
std::string name_;
std::unique_ptr<GuestOsTestVolume> volume_;
};

// GuestOsTestVolume: local test volume for the "Guest OS" directories.
Expand Down
4 changes: 3 additions & 1 deletion chrome/browser/ash/file_manager/path_util_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
#include "chrome/browser/ash/file_manager/volume_manager.h"
#include "chrome/browser/ash/file_manager/volume_manager_factory.h"
#include "chrome/browser/ash/file_system_provider/provided_file_system_info.h"
#include "chrome/browser/ash/guest_os/public/types.h"
#include "chrome/browser/ash/login/users/fake_chrome_user_manager.h"
#include "chrome/browser/ash/profiles/profile_helper.h"
#include "chrome/browser/chromeos/fileapi/file_system_backend.h"
Expand Down Expand Up @@ -1157,7 +1158,8 @@ TEST_F(FileManagerPathUtilTest, GetDisplayablePathTest) {

volume_manager->AddVolumeForTesting(Volume::CreateForSftpGuestOs(
"guest_os_label", base::FilePath("/mount_path/guest_os"),
base::FilePath("/remote_mount_path/guest_os")));
base::FilePath("/remote_mount_path/guest_os"),
guest_os::VmType::ApplicationList_VmType_TERMINA));

volume_manager->AddVolumeForTesting(Volume::CreateForMTP(
base::FilePath("/mount_path/mtp"), "mtp_label", false));
Expand Down
25 changes: 16 additions & 9 deletions chrome/browser/ash/file_manager/volume_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
#include "chrome/browser/ash/file_manager/volume_manager_factory.h"
#include "chrome/browser/ash/file_manager/volume_manager_observer.h"
#include "chrome/browser/ash/file_system_provider/provided_file_system_info.h"
#include "chrome/browser/ash/guest_os/public/types.h"
#include "chrome/browser/ash/profiles/profile_helper.h"
#include "chrome/browser/media_galleries/fileapi/mtp_device_map_service.h"
#include "chrome/browser/profiles/profile.h"
Expand Down Expand Up @@ -444,7 +445,8 @@ std::unique_ptr<Volume> Volume::CreateForSshfsCrostini(
std::unique_ptr<Volume> Volume::CreateForSftpGuestOs(
const std::string display_name,
const base::FilePath& sftp_mount_path,
const base::FilePath& remote_mount_path) {
const base::FilePath& remote_mount_path,
const guest_os::VmType vm_type) {
std::unique_ptr<Volume> volume(new Volume());
volume->type_ = VOLUME_TYPE_GUEST_OS;
volume->device_type_ = chromeos::DEVICE_TYPE_UNKNOWN;
Expand All @@ -456,6 +458,7 @@ std::unique_ptr<Volume> Volume::CreateForSftpGuestOs(
volume->volume_id_ = GenerateVolumeId(*volume);
volume->volume_label_ = display_name;
volume->watchable_ = false;
volume->vm_type_ = vm_type;
return volume;
}

Expand Down Expand Up @@ -805,10 +808,11 @@ void VolumeManager::AddSshfsCrostiniVolume(
void VolumeManager::AddSftpGuestOsVolume(
const std::string display_name,
const base::FilePath& sftp_mount_path,
const base::FilePath& remote_mount_path) {
const base::FilePath& remote_mount_path,
const guest_os::VmType vm_type) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
std::unique_ptr<Volume> volume = Volume::CreateForSftpGuestOs(
display_name, sftp_mount_path, remote_mount_path);
display_name, sftp_mount_path, remote_mount_path, vm_type);
// Ignore if volume already exists.
if (mounted_volumes_.find(volume->volume_id()) != mounted_volumes_.end())
return;
Expand Down Expand Up @@ -1695,12 +1699,15 @@ void VolumeManager::OnSftpGuestOsUnmountCallback(
(error_code == chromeos::MOUNT_ERROR_PATH_NOT_MOUNTED)) {
// Remove metadata associated with the mount. It will be a no-op if it
// wasn't mounted or unmounted out of band. We need the VolumeId to be
// consistent, which means the mount path needs to be the same. display_name
// and remote_mount_path aren't needed and we don't know them at unmount so
// leave them blank.
DoUnmountEvent(
chromeos::MOUNT_ERROR_NONE,
*Volume::CreateForSftpGuestOs("", sftp_mount_path, base::FilePath()));
// consistent, which means the mount path needs to be the same.
// display_name, remote_mount_path and vm_type aren't needed and we don't
// know them at unmount so leave them blank.
DoUnmountEvent(chromeos::MOUNT_ERROR_NONE,
// TODO(b/230667118): Once http://crrev/3627129 makes it into
// Chrome change the type to unknown.
*Volume::CreateForSftpGuestOs(
"", sftp_mount_path, base::FilePath(),
guest_os::VmType::ApplicationList_VmType_TERMINA));
if (callback)
std::move(callback).Run(true);
return;
Expand Down
14 changes: 12 additions & 2 deletions chrome/browser/ash/file_manager/volume_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

#include <map>
#include <memory>
#include <optional>
#include <string>
#include <vector>

Expand All @@ -24,11 +25,13 @@
#include "chrome/browser/ash/file_system_provider/observer.h"
#include "chrome/browser/ash/file_system_provider/provided_file_system_info.h"
#include "chrome/browser/ash/file_system_provider/service.h"
#include "chrome/browser/ash/guest_os/public/types.h"
#include "chromeos/dbus/cros_disks/cros_disks_client.h"
#include "components/keyed_service/core/keyed_service.h"
#include "components/prefs/pref_change_registrar.h"
#include "components/storage_monitor/removable_storage_observer.h"
#include "services/device/public/mojom/mtp_manager.mojom.h"
#include "third_party/abseil-cpp/absl/types/optional.h"

class Profile;

Expand Down Expand Up @@ -126,7 +129,8 @@ class Volume : public base::SupportsWeakPtr<Volume> {
static std::unique_ptr<Volume> CreateForSftpGuestOs(
const std::string display_name,
const base::FilePath& sftp_mount_path,
const base::FilePath& remote_mount_path);
const base::FilePath& remote_mount_path,
const guest_os::VmType vm_type);
static std::unique_ptr<Volume> CreateForAndroidFiles(
const base::FilePath& mount_path);
static std::unique_ptr<Volume> CreateForDocumentsProvider(
Expand Down Expand Up @@ -197,6 +201,8 @@ class Volume : public base::SupportsWeakPtr<Volume> {
}
bool hidden() const { return hidden_; }

absl::optional<guest_os::VmType> vm_type() const { return vm_type_; }

private:
Volume();

Expand Down Expand Up @@ -284,6 +290,9 @@ class Volume : public base::SupportsWeakPtr<Volume> {
// True if the volume is hidden and never shown to the user through File
// Manager.
bool hidden_;

// Only set for VOLUME_TYPE_GUEST_OS, identifies the type of Guest OS VM.
absl::optional<guest_os::VmType> vm_type_;
};

// Manages Volumes for file manager. Example of Volumes:
Expand Down Expand Up @@ -365,7 +374,8 @@ class VolumeManager : public KeyedService,
// removed on unmount (including Guest OS shutdown).
void AddSftpGuestOsVolume(const std::string display_name,
const base::FilePath& sftp_mount_path,
const base::FilePath& remote_mount_path);
const base::FilePath& remote_mount_path,
const guest_os::VmType vm_type);

// Removes specified sshfs crostini mount. Runs `callback` with true if the
// mount was removed successfully or wasn't mounted to begin with. Runs
Expand Down
5 changes: 5 additions & 0 deletions chrome/browser/ash/guest_os/guest_os_test_helpers.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// found in the LICENSE file.

#include "chrome/browser/ash/guest_os/guest_os_test_helpers.h"
#include "chrome/browser/ash/guest_os/public/types.h"

#ifndef CHROME_BROWSER_ASH_GUEST_OS_PUBLIC_GUEST_OS_TEST_HELPERS_H_
#define CHROME_BROWSER_ASH_GUEST_OS_PUBLIC_GUEST_OS_TEST_HELPERS_H_
Expand All @@ -28,6 +29,10 @@ crostini::ContainerId MockMountProvider::ContainerId() {
return container_id_;
}

VmType MockMountProvider::vm_type() {
return VmType::ApplicationList_VmType_PLUGIN_VM;
}

} // namespace guest_os

#endif // CHROME_BROWSER_ASH_GUEST_OS_PUBLIC_GUEST_OS_TEST_HELPERS_H_
3 changes: 3 additions & 0 deletions chrome/browser/ash/guest_os/guest_os_test_helpers.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

#include "chrome/browser/ash/crostini/crostini_util.h"
#include "chrome/browser/ash/guest_os/public/guest_os_mount_provider.h"
#include "chrome/browser/ash/guest_os/public/types.h"

namespace guest_os {
class MockMountProvider : public GuestOsMountProvider {
Expand All @@ -18,6 +19,8 @@ class MockMountProvider : public GuestOsMountProvider {
Profile* profile() override;
crostini::ContainerId ContainerId() override;

VmType vm_type() override;

Profile* profile_;
crostini::ContainerId container_id_;
};
Expand Down
17 changes: 11 additions & 6 deletions chrome/browser/ash/guest_os/public/guest_os_mount_provider.cc
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ class ScopedVolume {
std::string display_name,
std::string mount_label,
base::FilePath homedir,
const ash::disks::DiskMountManager::MountPointInfo& mount_info)
const ash::disks::DiskMountManager::MountPointInfo& mount_info,
VmType vm_type)
: profile_(profile), mount_label_(mount_label) {
base::FilePath mount_path = base::FilePath(mount_info.mount_path);
if (!storage::ExternalMountPoints::GetSystemInstance()->RegisterFileSystem(
Expand All @@ -44,7 +45,7 @@ class ScopedVolume {
auto* vmgr = file_manager::VolumeManager::Get(profile_);
if (vmgr) {
// vmgr is null in unit tests.
vmgr->AddSftpGuestOsVolume(display_name, mount_path, homedir);
vmgr->AddSftpGuestOsVolume(display_name, mount_path, homedir, vm_type);
}
}

Expand Down Expand Up @@ -81,13 +82,15 @@ class GuestOsMountProviderInner : public CachedCallback<ScopedVolume, bool> {
crostini::ContainerId container_id,
int cid,
int port,
base::FilePath homedir)
base::FilePath homedir,
VmType vm_type)
: profile_(profile),
display_name_(display_name),
container_id_(container_id),
cid_(cid),
port_(port),
homedir_(homedir) {}
homedir_(homedir),
vm_type_(vm_type) {}

// Mount.
void Build(RealCallback callback) override {
Expand Down Expand Up @@ -118,7 +121,7 @@ class GuestOsMountProviderInner : public CachedCallback<ScopedVolume, bool> {
return;
}
auto scoped_volume = std::make_unique<ScopedVolume>(
profile_, display_name_, mount_label_, homedir_, mount_info);
profile_, display_name_, mount_label_, homedir_, mount_info, vm_type_);

// CachedCallback magic keeps the scope alive until we're destroyed or it's
// invalidated.
Expand All @@ -132,6 +135,7 @@ class GuestOsMountProviderInner : public CachedCallback<ScopedVolume, bool> {
int cid_;
int port_; // vsock port
base::FilePath homedir_;
VmType vm_type_;

// Note: This should remain the last member so it'll be destroyed and
// invalidate its weak pointers before any other members are destroyed.
Expand All @@ -141,7 +145,8 @@ class GuestOsMountProviderInner : public CachedCallback<ScopedVolume, bool> {
void GuestOsMountProvider::Mount(base::OnceCallback<void(bool)> callback) {
if (!callback_) {
callback_ = std::make_unique<GuestOsMountProviderInner>(
profile(), DisplayName(), ContainerId(), cid(), port(), homedir());
profile(), DisplayName(), ContainerId(), cid(), port(), homedir(),
vm_type());
}
callback_->Get(base::BindOnce(
[](base::OnceCallback<void(bool)> callback,
Expand Down
6 changes: 6 additions & 0 deletions chrome/browser/ash/guest_os/public/guest_os_mount_provider.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "base/callback_forward.h"
#include "base/files/file_path.h"
#include "chrome/browser/ash/crostini/crostini_util.h"
#include "chrome/browser/ash/guest_os/public/types.h"

class Profile;

Expand Down Expand Up @@ -41,6 +42,11 @@ class GuestOsMountProvider {
virtual int port();
virtual base::FilePath homedir();

// The type of VM which this provider creates mounts for. Needed for e.g.
// enterprise policy which applies different rules to each disk volume
// depending on the underlying VM.
virtual VmType vm_type() = 0;

// Requests the provider to mount its volume.
void Mount(base::OnceCallback<void(bool)> callback);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ TEST_F(GuestOsMountProviderTest, MountDiskMountsDisk) {
ASSERT_TRUE(volume);
EXPECT_EQ(volume->type(), file_manager::VOLUME_TYPE_GUEST_OS);
EXPECT_EQ(volume->volume_label(), provider_->DisplayName());
EXPECT_EQ(volume->vm_type(), provider_->vm_type());
}

TEST_F(GuestOsMountProviderTest, MultipleCallsAreQueuedAndOnlyMountOnce) {
Expand Down
14 changes: 14 additions & 0 deletions chrome/browser/ash/guest_os/public/types.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// Copyright 2022 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#ifndef CHROME_BROWSER_ASH_GUEST_OS_PUBLIC_TYPES_H_
#define CHROME_BROWSER_ASH_GUEST_OS_PUBLIC_TYPES_H_

#include "chromeos/dbus/vm_applications/apps.pb.h"
namespace guest_os {

using VmType = vm_tools::apps::ApplicationList_VmType;
}

#endif // CHROME_BROWSER_ASH_GUEST_OS_PUBLIC_TYPES_H_
1 change: 1 addition & 0 deletions chrome/browser/chromeos/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -1659,6 +1659,7 @@ source_set("chromeos") {
"../ash/guest_os/public/guest_os_service_factory.h",
"../ash/guest_os/public/guest_os_wayland_server.cc",
"../ash/guest_os/public/guest_os_wayland_server.h",
"../ash/guest_os/public/types.h",
"../ash/guest_os/virtual_machines/virtual_machines_util.cc",
"../ash/guest_os/virtual_machines/virtual_machines_util.h",
"../ash/guest_os/vm_sk_forwarding_native_message_host.cc",
Expand Down

0 comments on commit 5e249db

Please sign in to comment.