From 025f6c90d808d56c4c16c932465093e959b75e78 Mon Sep 17 00:00:00 2001 From: Alan Somers Date: Thu, 28 Sep 2017 16:51:39 -0600 Subject: [PATCH] blkdev: Add dedicated accessors for all property functions Hide get_block_device_string_property and get_block_device_int_property to lower risk of misuse. Signed-off-by: Alan Somers --- src/common/blkdev.cc | 40 ++++++++++++++++++++++++++++++-- src/common/blkdev.h | 9 ++++--- src/os/bluestore/KernelDevice.cc | 29 ++++++++++------------- src/os/bluestore/PMEMDevice.cc | 29 ++++++++++------------- src/test/common/test_blkdev.cc | 23 ++---------------- 5 files changed, 68 insertions(+), 62 deletions(-) diff --git a/src/common/blkdev.cc b/src/common/blkdev.cc index 197cf895120..0534a4d8592 100644 --- a/src/common/blkdev.cc +++ b/src/common/blkdev.cc @@ -174,7 +174,7 @@ int get_block_device_base(const char *dev, char *out, size_t out_len) * return 0 on success * return negative error on error */ -int64_t get_block_device_string_property(const char *devname, +static int64_t get_block_device_string_property(const char *devname, blkdev_prop_t prop, char *val, size_t maxlen) { @@ -211,7 +211,7 @@ int64_t get_block_device_string_property(const char *devname, * return the value (we assume it is positive) * return negative error on error */ -int64_t get_block_device_int_property(const char *devname, blkdev_prop_t prop) +static int64_t get_block_device_int_property(const char *devname, blkdev_prop_t prop) { char buff[256] = {0}; int r = get_block_device_string_property(devname, prop, buff, sizeof(buff)); @@ -242,6 +242,15 @@ int block_device_discard(int fd, int64_t offset, int64_t len) return ioctl(fd, BLKDISCARD, range); } +bool block_device_is_nvme(const char *devname) +{ + char vendor[80]; + // nvme has a device/device/vendor property; infer from that. There is + // probably a better way? + int r = get_block_device_string_property(devname, BLKDEV_PROP_VENDOR, vendor, 80); + return (r > 0); +} + bool block_device_is_rotational(const char *devname) { return get_block_device_int_property(devname, BLKDEV_PROP_ROTATIONAL) > 0; @@ -252,6 +261,11 @@ int block_device_vendor(const char *devname, char *vendor, size_t max) return get_block_device_string_property(devname, BLKDEV_PROP_VENDOR, vendor, max); } +int block_device_dev(const char *devname, char *dev, size_t max) +{ + return get_block_device_string_property(devname, BLKDEV_PROP_DEV, dev, max); +} + int block_device_model(const char *devname, char *model, size_t max) { return get_block_device_string_property(devname, BLKDEV_PROP_MODEL, model, @@ -259,6 +273,13 @@ int block_device_model(const char *devname, char *model, size_t max) } int block_device_serial(const char *devname, char *serial, size_t max) +{ + return get_block_device_string_property(devname, BLKDEV_PROP_SERIAL, serial, + max); +} + +int get_device_by_uuid(uuid_d dev_uuid, const char* label, char* partition, + char* device) { return get_block_device_string_property(devname, BLKDEV_PROP_SERIAL, serial, max); } @@ -536,6 +557,11 @@ int block_device_discard(int fd, int64_t offset, int64_t len) return -EOPNOTSUPP; } +bool block_device_is_nvme(const char *devname) +{ + return false; +} + bool block_device_is_rotational(const char *devname) { return false; @@ -581,6 +607,11 @@ int block_device_discard(int fd, int64_t offset, int64_t len) return -EOPNOTSUPP; } +bool block_device_is_nvme(const char *devname) +{ + return false; +} + bool block_device_is_rotational(const char *devname) { return false; @@ -638,6 +669,11 @@ int block_device_discard(int fd, int64_t offset, int64_t len) return -EOPNOTSUPP; } +bool block_device_is_nvme(const char *devname) +{ + return false; +} + bool block_device_is_rotational(const char *devname) { return false; diff --git a/src/common/blkdev.h b/src/common/blkdev.h index a2569c23fc9..81525ea6634 100644 --- a/src/common/blkdev.h +++ b/src/common/blkdev.h @@ -27,12 +27,8 @@ extern int get_device_by_path(const char *path, char* partition, char* device, s extern int get_device_by_fd(int fd, char* partition, char* device, size_t max); // from a device (e.g., "sdb") -extern int64_t get_block_device_int_property( - const char *devname, blkdev_prop_t prop); -extern int64_t get_block_device_string_property( - const char *devname, blkdev_prop_t prop, - char *val, size_t maxlen); extern bool block_device_support_discard(const char *devname); +extern bool block_device_is_nvme(const char *devname); extern bool block_device_is_rotational(const char *devname); extern int block_device_vendor(const char *devname, char *vendor, size_t max); extern int block_device_model(const char *devname, char *model, size_t max); @@ -50,5 +46,8 @@ extern int block_device_run_smartctl(const char *device, int timeout, extern int get_vdo_stats_handle(const char *devname, std::string *vdo_name); extern int64_t get_vdo_stat(int fd, const char *property); extern bool get_vdo_utilization(int fd, uint64_t *total, uint64_t *avail); +extern int block_device_dev(const char *devname, char *dev, size_t max); +extern int block_device_model(const char *devname, char *model, size_t max); +extern int block_device_serial(const char *devname, char *serial, size_t max); #endif diff --git a/src/os/bluestore/KernelDevice.cc b/src/os/bluestore/KernelDevice.cc index 0bf470c537f..5f86a439a0a 100644 --- a/src/os/bluestore/KernelDevice.cc +++ b/src/os/bluestore/KernelDevice.cc @@ -209,13 +209,6 @@ void KernelDevice::close() path.clear(); } -static string get_dev_property(const char *dev, blkdev_prop_t property) -{ - char val[1024] = {0}; - get_block_device_string_property(dev, property, val, sizeof(val)); - return val; -} - int KernelDevice::collect_metadata(const string& prefix, map *pm) const { (*pm)[prefix + "rotational"] = stringify((int)(bool)rotational); @@ -255,21 +248,23 @@ int KernelDevice::collect_metadata(const string& prefix, map *pm) break; default: { + char buffer[1024] = {0}; (*pm)[prefix + "partition_path"] = string(partition_path); (*pm)[prefix + "dev_node"] = string(dev_node); - (*pm)[prefix + "model"] = get_dev_property(dev_node, BLKDEV_PROP_MODEL); - (*pm)[prefix + "dev"] = get_dev_property(dev_node, BLKDEV_PROP_DEV); + block_device_model(dev_node, buffer, sizeof(buffer)); + (*pm)[prefix + "model"] = buffer; + + buffer[0] = '\0'; + block_device_dev(dev_node, buffer, sizeof(buffer)); + (*pm)[prefix + "dev"] = buffer; + // nvme exposes a serial number - string serial = get_dev_property(dev_node, BLKDEV_PROP_SERIAL); - if (serial.length()) { - (*pm)[prefix + "serial"] = serial; - } + buffer[0] = '\0'; + block_device_serial(dev_node, buffer, sizeof(buffer)); + (*pm)[prefix + "serial"] = buffer; - // nvme has a device/device/* structure; infer from that. there - // is probably a better way? - string nvme_vendor = get_dev_property(dev_node, BLKDEV_PROP_VENDOR); - if (nvme_vendor.length()) { + if (block_device_is_nvme(dev_node)) { (*pm)[prefix + "type"] = "nvme"; } } diff --git a/src/os/bluestore/PMEMDevice.cc b/src/os/bluestore/PMEMDevice.cc index a711c84a7eb..ee6bf771508 100644 --- a/src/os/bluestore/PMEMDevice.cc +++ b/src/os/bluestore/PMEMDevice.cc @@ -128,13 +128,6 @@ void PMEMDevice::close() path.clear(); } -static string get_dev_property(const char *dev, blkdev_prop_t property) -{ - char val[1024] = {0}; - get_block_device_string_property(dev, property, val, sizeof(val)); - return val; -} - int PMEMDevice::collect_metadata(const string& prefix, map *pm) const { (*pm)[prefix + "rotational"] = stringify((int)(bool)rotational); @@ -164,21 +157,23 @@ int PMEMDevice::collect_metadata(const string& prefix, map *pm) c break; default: { + char buffer[1024] = {0}; (*pm)[prefix + "partition_path"] = string(partition_path); (*pm)[prefix + "dev_node"] = string(dev_node); - (*pm)[prefix + "model"] = get_dev_property(dev_node, BLKDEV_PROP_MODEL); - (*pm)[prefix + "dev"] = get_dev_property(dev_node, BLKDEV_PROP_DEV); + block_device_model(dev_node, buffer, sizeof(buffer)); + (*pm)[prefix + "model"] = buffer; + + buffer[0] = '\0'; + block_device_dev(dev_node, buffer, sizeof(buffer)); + (*pm)[prefix + "dev"] = buffer; + // nvme exposes a serial number - string serial = get_dev_property(dev_node, BLKDEV_PROP_SERIAL); - if (serial.length()) { - (*pm)[prefix + "serial"] = serial; - } + buffer[0] = '\0'; + block_device_serial(dev_node, buffer, sizeof(buffer)); + (*pm)[prefix + "serial"] = buffer; - // nvme has a device/device/* structure; infer from that. there - // is probably a better way? - string nvme_vendor = get_dev_property(dev_node, BLKDEV_PROP_VENDOR); - if (nvme_vendor.length()) { + if (block_device_nvme(dev_node)) { (*pm)[prefix + "type"] = "nvme"; } } diff --git a/src/test/common/test_blkdev.cc b/src/test/common/test_blkdev.cc index fbca85c37c0..625b84bca05 100644 --- a/src/test/common/test_blkdev.cc +++ b/src/test/common/test_blkdev.cc @@ -48,8 +48,7 @@ TEST(blkdev, get_block_device_base) { ASSERT_EQ(0, get_block_device_base(base, buf3, sizeof(buf3))); printf(" got '%s' expected '%s'\n", buf3, de->d_name); ASSERT_EQ(0, strcmp(de->d_name, buf3)); - printf(" discard granularity = %lld .. supported = %d\n", - (long long)get_block_device_int_property(base, BLKDEV_PROP_DISCARD_GRANULARITY), + printf(" discard supported = %d\n", (int)block_device_support_discard(base)); char subdirfn[PATH_MAX]; @@ -75,8 +74,7 @@ TEST(blkdev, get_block_device_base) { ASSERT_EQ(0, get_block_device_base(part, buf3, sizeof(buf3))); printf(" got '%s' expected '%s'\n", buf3, de->d_name); ASSERT_EQ(0, strcmp(buf3, de->d_name)); - printf(" discard granularity = %lld .. supported = %d\n", - (long long)get_block_device_int_property(part, BLKDEV_PROP_DISCARD_GRANULARITY), + printf(" discard supported = %d\n", (int)block_device_support_discard(part)); } @@ -101,21 +99,6 @@ TEST(blkdev, device_model) ASSERT_EQ(strcmp(model, "myfancymodel"), 0); } -TEST(blkdev, get_block_device_string_property) -{ - const char* env = getenv("CEPH_ROOT"); - ASSERT_NE(env, nullptr) << "Environment Variable CEPH_ROOT not found!"; - string root = string(env) + "/src/test/common/test_blkdev_sys_block"; - set_block_device_sandbox_dir(root.c_str()); - - char val[1000] = {0}; - int rc = get_block_device_string_property("sda", BLKDEV_PROP_MODEL, - val, sizeof(val)); - ASSERT_EQ(0, rc); - printf("val '%s'\n", val); - ASSERT_EQ(strcmp(val, "myfancymodel"), 0); -} - TEST(blkdev, is_rotational) { const char* env = getenv("CEPH_ROOT"); @@ -126,5 +109,3 @@ TEST(blkdev, is_rotational) ASSERT_FALSE(block_device_is_rotational("sda")); ASSERT_TRUE(block_device_is_rotational("sdb")); } - - -- 2.39.5