Re: [RFC PATCH 01/21] block: add and use init tagset helper

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 10/5/22 02:47, Ulf Hansson wrote:
On Wed, 5 Oct 2022 at 07:11, Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx> wrote:
On 10/5/22 12:22, Chaitanya Kulkarni wrote:
+void blk_mq_init_tag_set(struct blk_mq_tag_set *set,
+             const struct blk_mq_ops *ops, unsigned int nr_hw_queues,
+             unsigned int queue_depth, unsigned int cmd_size, int numa_node,
+             unsigned int timeout, unsigned int flags, void *driver_data)

That is an awful lot of arguments... I would be tempted to say pack all
these into a struct but then that would kind of negate this patchset goal.
Using a function with that many arguments will be error prone, and hard to
review... Not a fan.

I completely agree.

But there is also another problem going down this route. If/when we
realize that there is another parameter needed in the blk_mq_tag_set.
Today that's quite easy to add (assuming the parameter can be
optional), without changing the blk_mq_init_tag_set() interface.

Hi Chaitanya,

Please consider to drop the entire patch series. In addition to the disadvantages mentioned above I'd like to mention the following disadvantages:
* Replacing named member assignments with positional arguments in a
  function call makes code harder to read and harder to verify.
* This patch series makes tree-wide changes without improving the code
  in a substantial way.

Thanks,

Bart.

_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization



[Index of Archives]     [KVM Development]     [Libvirt Development]     [Libvirt Users]     [CentOS Virtualization]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux