Re: [PATCH 2/7] cbd: introduce cbd_transport

[Dongsheng Yang]

在 2024/4/24 星期三 下午 12:08, Chaitanya Kulkarni 写道:
> > +static ssize_t cbd_myhost_show(struct device *dev,
> > +			       struct device_attribute *attr,
> > +			       char *buf)
> > +{
> > +	struct cbd_transport *cbdt;
> > +	struct cbd_host *host;
> > +
> > +	cbdt = container_of(dev, struct cbd_transport, device);
> > +
> > +	host = cbdt->host;
> > +	if (!host)
> > +		return 0;
> > +
> > +	return sprintf(buf, "%d\n", host->host_id);
>
> snprintf() ?

IMO, it will only print a decimal unsigned int, so it shouldn't overflow 
the buffer.
> > +}
> > +
> > +static DEVICE_ATTR(my_host_id, 0400, cbd_myhost_show, NULL);
> > +
> > +enum {
>
> [...]
>
> > +
> > +static ssize_t cbd_adm_store(struct device *dev,
> > +				 struct device_attribute *attr,
> > +				 const char *ubuf,
> > +				 size_t size)
> > +{
> > +	int ret;
> > +	char *buf;
> > +	struct cbd_adm_options opts = { 0 };
> > +	struct cbd_transport *cbdt;
> > +
>
> reverse tree order that matches rest of your code ?

Agreed,
> > +	if (!capable(CAP_SYS_ADMIN))
> > +		return -EPERM;
> > +
> > +	cbdt = container_of(dev, struct cbd_transport, device);
> > +
> > +	buf = kmemdup(ubuf, size + 1, GFP_KERNEL);
> > +	if (IS_ERR(buf)) {
> > +		pr_err("failed to dup buf for adm option: %d", (int)PTR_ERR(buf));
> > +		return PTR_ERR(buf);
> > +	}
> > +	buf[size] = '\0';
> > +	ret = parse_adm_options(cbdt, buf, &opts);
> > +	if (ret < 0) {
> > +		kfree(buf);
> > +		return ret;
> > +	}
> > +	kfree(buf);
> > +
>
> standard format is using goto out and having only on kfree()
> at the end of the function ...

Okey, having a unified error handling path is a good idea, and it's 
suitable here as well, thanx.
> > +	switch (opts.op) {
> > +	case CBDT_ADM_OP_B_START:
> > +		break;
> > +	case CBDT_ADM_OP_B_STOP:
> > +		break;
> > +	case CBDT_ADM_OP_B_CLEAR:
> > +		break;
> > +	case CBDT_ADM_OP_DEV_START:
> > +		break;
> > +	case CBDT_ADM_OP_DEV_STOP:
> > +		break;
> > +	default:
> > +		pr_err("invalid op: %d\n", opts.op);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return size;
> > +}
> > +
>
> [...]
>
> > +static struct cbd_transport *cbdt_alloc(void)
> > +{
> > +	struct cbd_transport *cbdt;
> > +	int ret;
> > +
> > +	cbdt = kzalloc(sizeof(struct cbd_transport), GFP_KERNEL);
> > +	if (!cbdt) {
> > +		return NULL;
> > +	}
>
> no braces needed for single statements in if ... applies rest of
> the code ...

thanx, Iwill remove unnecessary braces next version.
> -ck