On 4/19/20 12:45 PM, Luis Chamberlain wrote:
+int __must_check blk_queue_debugfs_register(struct request_queue *q)
+{
+ struct dentry *dir = NULL;
+
+ /* This can happen if we have a bug in the lower layers */
What does "this" refer to? Which layers does "lower layers" refer to?
Most software developers consider a module that calls directly into
another module as a higher layer (callbacks through function pointers do
not count; see also https://en.wikipedia.org/wiki/Modular_programming).
According to that definition block drivers are a software layer
immediately above the block layer core.
How about changing that comment into the following to make it
unambiguous (if this is what you meant)?
/*
* Check whether the debugfs directory already exists. This can
* only happen as the result of a bug in a block driver.
*/
+ dir = debugfs_lookup(kobject_name(q->kobj.parent), blk_debugfs_root);
+ if (dir) {
+ pr_warn("%s: registering request_queue debugfs directory twice is not allowed\n",
+ kobject_name(q->kobj.parent));
+ dput(dir);
+ return -EALREADY;
+ }
+
+ q->debugfs_dir = debugfs_create_dir(kobject_name(q->kobj.parent),
+ blk_debugfs_root);
+ if (!q->debugfs_dir)
+ return -ENOMEM;
+
+ return 0;
+}
kobject_name(q->kobj.parent) is used three times in the above function.
How about introducing a local variable that holds the result of that
expression?
+static bool blk_trace_target_disk(const char *target, const char *diskname)
+{
+ if (strlen(target) != strlen(diskname))
+ return false;
+
+ if (!strncmp(target, diskname,
+ min_t(size_t, strlen(target), strlen(diskname))))
+ return true;
+
+ return false;
+}
The above code looks weird to me. When the second if-statement is
reached, it is guaranteed that 'target' and 'diskname' have the same
length. So why to calculate the minimum length in the second
if-statement of two strings that have the same length?
Independent of what the purpose of the above code is, can that code be
rewritten such that it does not depend on the details of how names are
assigned to disks and partitions? Would disk_get_part() be useful here?
Thanks,
Bart.
