Messages in this thread | | | Subject | Re: [PATCH 2/7] cbd: introduce cbd_transport | From | Dongsheng Yang <> | Date | Wed, 24 Apr 2024 16:43:47 +0800 |
| |
在 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 > >
| |