Re: [PATCH v5] Staging: exfat: Avoid use of strcpy
> On 12 Sep 2019, at 10:34, Dan Carpenter wrote: > > You did it. Well done. :P > > Reviewed-by: Dan Carpenter Thanks :D Had some issues with my git configuration setting up a home workstation but now it is all fine
[PATCH v5] Staging: exfat: Avoid use of strcpy
Use strscpy instead of strcpy in exfat_core.c, and add a check for length that will return already known FFS_INVALIDPATH. Suggested-by: Rasmus Villemoes Signed-off-by: Sandro Volery --- v5: Fixed some whitespaces v4: Replaced strlen check v3: Failed to replace check v2: Forgot to replace strlen check v1: original patch drivers/staging/exfat/exfat_core.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/staging/exfat/exfat_core.c b/drivers/staging/exfat/exfat_core.c index da8c58149c35..ee474ae3bd98 100644 --- a/drivers/staging/exfat/exfat_core.c +++ b/drivers/staging/exfat/exfat_core.c @@ -2961,11 +2961,9 @@ s32 resolve_path(struct inode *inode, char *path, struct chain_t *p_dir, struct fs_info_t *p_fs = &(EXFAT_SB(sb)->fs_info); struct file_id_t *fid = &(EXFAT_I(inode)->fid); - if (strlen(path) >= (MAX_NAME_LENGTH * MAX_CHARSET_SIZE)) + if (strscpy(name_buf, path, sizeof(name_buf)) < 0) return FFS_INVALIDPATH; - strcpy(name_buf, path); - nls_cstring_to_uniname(sb, p_uniname, name_buf, ); if (lossy) return FFS_INVALIDPATH; -- 2.23.0
Re: [PATCH v4] Staging: exfat: avoid use of strcpy
> On 11 Sep 2019, at 21:06, Dan Carpenter wrote: > > On Wed, Sep 11, 2019 at 09:53:03PM +0200, Sandro Volery wrote: >> diff --git a/drivers/staging/exfat/exfat_core.c >> b/drivers/staging/exfat/exfat_core.c >> index da8c58149c35..4336fee444ce 100644 >> --- a/drivers/staging/exfat/exfat_core.c >> +++ b/drivers/staging/exfat/exfat_core.c >> @@ -2960,18 +2960,15 @@ s32 resolve_path(struct inode *inode, char *path, >> struct chain_t *p_dir, >>struct super_block *sb = inode->i_sb; >>struct fs_info_t *p_fs = &(EXFAT_SB(sb)->fs_info); >>struct file_id_t *fid = &(EXFAT_I(inode)->fid); >> - >> -if (strlen(path) >= (MAX_NAME_LENGTH * MAX_CHARSET_SIZE)) >> + > > You have added a tab here. > >> +if (strscpy(name_buf, path, sizeof(name_buf)) < 0) >>return FFS_INVALIDPATH; >> >> -strcpy(name_buf, path); >> - >>nls_cstring_to_uniname(sb, p_uniname, name_buf, ); >>if (lossy) >>return FFS_INVALIDPATH; >> >> -fid->size = i_size_read(inode); >> - >> +fid->size = i_size_read(inode); > > And you accidentally deleted some white space here. > > I use vim, so I have it configured to highlight whitespace at the end of > a line. I don't remember how it's done now but I googled it for you. > https://vim.fandom.com/wiki/Highlight_unwanted_spaces Ugh I'm so sorry I make the most stupid mistakes today.. I was so sure this time I got it right! I'll fix it tomorrow. Thanks, Sandro V
[PATCH v4] Staging: exfat: avoid use of strcpy
Replacing strcpy with strscpy and moving the length check to the same function. Suggested-by: Rasmus Villemoes Signed-off-by: Sandro Volery --- Took a couple attempts to finaly get this right :P v4: Replaced strlen check v3: Failed to replace check v2: Forgot to replace strlen check v1: original patch drivers/staging/exfat/exfat_core.c | 9 +++-- 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/drivers/staging/exfat/exfat_core.c b/drivers/staging/exfat/exfat_core.c index da8c58149c35..4336fee444ce 100644 --- a/drivers/staging/exfat/exfat_core.c +++ b/drivers/staging/exfat/exfat_core.c @@ -2960,18 +2960,15 @@ s32 resolve_path(struct inode *inode, char *path, struct chain_t *p_dir, struct super_block *sb = inode->i_sb; struct fs_info_t *p_fs = &(EXFAT_SB(sb)->fs_info); struct file_id_t *fid = &(EXFAT_I(inode)->fid); - - if (strlen(path) >= (MAX_NAME_LENGTH * MAX_CHARSET_SIZE)) + + if (strscpy(name_buf, path, sizeof(name_buf)) < 0) return FFS_INVALIDPATH; - strcpy(name_buf, path); - nls_cstring_to_uniname(sb, p_uniname, name_buf, ); if (lossy) return FFS_INVALIDPATH; - fid->size = i_size_read(inode); - +fid->size = i_size_read(inode); p_dir->dir = fid->start_clu; p_dir->size = (s32)(fid->size >> p_fs->cluster_size_bits); p_dir->flags = fid->flags; -- 2.23.0
[PATCH v3] Staging: exfat: Avoid use of strcpy
Use strscpy instead of strcpy in exfat_core.c, and add a check for length that will return already known FFS_INVALIDPATH. Suggested-by: Rasmus Villemoes Signed-off-by: Sandro Volery --- v3: Fixed replacing mistake v2: Introduced length check v1: Original patch drivers/staging/exfat/exfat_core.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/staging/exfat/exfat_core.c b/drivers/staging/exfat/exfat_core.c index da8c58149c35..4c40f1352848 100644 --- a/drivers/staging/exfat/exfat_core.c +++ b/drivers/staging/exfat/exfat_core.c @@ -2964,7 +2964,8 @@ s32 resolve_path(struct inode *inode, char *path, struct chain_t *p_dir, if (strlen(path) >= (MAX_NAME_LENGTH * MAX_CHARSET_SIZE)) return FFS_INVALIDPATH; - strcpy(name_buf, path); + if (strscpy(name_buf, path, sizeof(name_buf)) < 0) + return FFS_INVALIDPATH; nls_cstring_to_uniname(sb, p_uniname, name_buf, ); if (lossy) -- 2.23.0
Re: [PATCH v2] Staging: exfat: Avoid use of strcpy
> On 11 Sep 2019, at 12:06, Dan Carpenter wrote: > > On Wed, Sep 11, 2019 at 11:42:19AM +0200, Sandro Volery wrote: >> Use strscpy instead of strcpy in exfat_core.c, and add a check >> for length that will return already known FFS_INVALIDPATH. >> >> Suggested-by: Rasmus Villemoes >> Signed-off-by: Sandro Volery >> --- >> v2: Implement length check and return in one >> v1: Original Patch >> drivers/staging/exfat/exfat_core.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/staging/exfat/exfat_core.c >> b/drivers/staging/exfat/exfat_core.c >> index da8c58149c35..4c40f1352848 100644 >> --- a/drivers/staging/exfat/exfat_core.c >> +++ b/drivers/staging/exfat/exfat_core.c >> @@ -2964,7 +2964,8 @@ s32 resolve_path(struct inode *inode, char *path, >> struct chain_t *p_dir, >>if (strlen(path) >= (MAX_NAME_LENGTH * MAX_CHARSET_SIZE)) > > Delete this as it is no longer required. > Yep, saw it from Rasmus response too just now.. Dumb mistake. Will fix this this afternoon. Sandro V >>return FFS_INVALIDPATH; >> >> -strcpy(name_buf, path); >> +if (strscpy(name_buf, path, sizeof(name_buf)) < 0) >> +return FFS_INVALIDPATH; >
[PATCH v2] Staging: exfat: Avoid use of strcpy
Use strscpy instead of strcpy in exfat_core.c, and add a check for length that will return already known FFS_INVALIDPATH. Suggested-by: Rasmus Villemoes Signed-off-by: Sandro Volery --- v2: Implement length check and return in one v1: Original Patch drivers/staging/exfat/exfat_core.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/staging/exfat/exfat_core.c b/drivers/staging/exfat/exfat_core.c index da8c58149c35..4c40f1352848 100644 --- a/drivers/staging/exfat/exfat_core.c +++ b/drivers/staging/exfat/exfat_core.c @@ -2964,7 +2964,8 @@ s32 resolve_path(struct inode *inode, char *path, struct chain_t *p_dir, if (strlen(path) >= (MAX_NAME_LENGTH * MAX_CHARSET_SIZE)) return FFS_INVALIDPATH; - strcpy(name_buf, path); + if (strscpy(name_buf, path, sizeof(name_buf)) < 0) + return FFS_INVALIDPATH; nls_cstring_to_uniname(sb, p_uniname, name_buf, ); if (lossy) -- 2.23.0
Re: [PATCH] Staging: octeon: Avoid several usecases of strcpy
On 11 Sep 2019, at 11:17, Dan Carpenter wrote: > > On Wed, Sep 11, 2019 at 11:04:38AM +0200, Sandro Volery wrote: >> >> >>>> On 11 Sep 2019, at 10:52, Dan Carpenter wrote: >>> >>> On Wed, Sep 11, 2019 at 08:23:59AM +0200, Sandro Volery wrote: >>>> strcpy was used multiple times in strcpy to write into dev->name. >>>> I replaced them with strscpy. >>>> >>>> Signed-off-by: Sandro Volery >>>> --- >>>> drivers/staging/octeon/ethernet.c | 16 >>>> 1 file changed, 8 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/drivers/staging/octeon/ethernet.c >>>> b/drivers/staging/octeon/ethernet.c >>>> index 8889494adf1f..cf8e9a23ebf9 100644 >>>> --- a/drivers/staging/octeon/ethernet.c >>>> +++ b/drivers/staging/octeon/ethernet.c >>>> @@ -784,7 +784,7 @@ static int cvm_oct_probe(struct platform_device *pdev) >>>> priv->imode = CVMX_HELPER_INTERFACE_MODE_DISABLED; >>>> priv->port = CVMX_PIP_NUM_INPUT_PORTS; >>>> priv->queue = -1; >>>> -strcpy(dev->name, "pow%d"); >>>> +strscpy(dev->name, "pow%d", sizeof(dev->name)); >>> >>> Is there a program which is generating a warning for this code? We know >>> that "pow%d" is 6 characters and static analysis tools can understand >>> this code fine so we know it's safe. >> >> Well I was confused too but checkpatch complained about >> it so I figured I'd clean it up quick > > Ah. It's a new checkpatch warning. I don't care in that case. I'm > fine with replacing all of these in that case. Alright thanks. Can you review this? Thanks, Sandro V
Re: [PATCH] Staging: octeon: Avoid several usecases of strcpy
> On 11 Sep 2019, at 10:52, Dan Carpenter wrote: > > On Wed, Sep 11, 2019 at 08:23:59AM +0200, Sandro Volery wrote: >> strcpy was used multiple times in strcpy to write into dev->name. >> I replaced them with strscpy. >> >> Signed-off-by: Sandro Volery >> --- >> drivers/staging/octeon/ethernet.c | 16 >> 1 file changed, 8 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/staging/octeon/ethernet.c >> b/drivers/staging/octeon/ethernet.c >> index 8889494adf1f..cf8e9a23ebf9 100644 >> --- a/drivers/staging/octeon/ethernet.c >> +++ b/drivers/staging/octeon/ethernet.c >> @@ -784,7 +784,7 @@ static int cvm_oct_probe(struct platform_device *pdev) >>priv->imode = CVMX_HELPER_INTERFACE_MODE_DISABLED; >>priv->port = CVMX_PIP_NUM_INPUT_PORTS; >>priv->queue = -1; >> -strcpy(dev->name, "pow%d"); >> +strscpy(dev->name, "pow%d", sizeof(dev->name)); > > Is there a program which is generating a warning for this code? We know > that "pow%d" is 6 characters and static analysis tools can understand > this code fine so we know it's safe. Well I was confused too but checkpatch complained about it so I figured I'd clean it up quick
[PATCH] Staging: octeon: Avoid several usecases of strcpy
strcpy was used multiple times in strcpy to write into dev->name. I replaced them with strscpy. Signed-off-by: Sandro Volery --- drivers/staging/octeon/ethernet.c | 16 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/staging/octeon/ethernet.c b/drivers/staging/octeon/ethernet.c index 8889494adf1f..cf8e9a23ebf9 100644 --- a/drivers/staging/octeon/ethernet.c +++ b/drivers/staging/octeon/ethernet.c @@ -784,7 +784,7 @@ static int cvm_oct_probe(struct platform_device *pdev) priv->imode = CVMX_HELPER_INTERFACE_MODE_DISABLED; priv->port = CVMX_PIP_NUM_INPUT_PORTS; priv->queue = -1; - strcpy(dev->name, "pow%d"); + strscpy(dev->name, "pow%d", sizeof(dev->name)); for (qos = 0; qos < 16; qos++) skb_queue_head_init(>tx_free_list[qos]); dev->min_mtu = VLAN_ETH_ZLEN - mtu_overhead; @@ -856,39 +856,39 @@ static int cvm_oct_probe(struct platform_device *pdev) case CVMX_HELPER_INTERFACE_MODE_NPI: dev->netdev_ops = _oct_npi_netdev_ops; - strcpy(dev->name, "npi%d"); + strscpy(dev->name, "npi%d", sizeof(dev->name)); break; case CVMX_HELPER_INTERFACE_MODE_XAUI: dev->netdev_ops = _oct_xaui_netdev_ops; - strcpy(dev->name, "xaui%d"); + strscpy(dev->name, "xaui%d", sizeof(dev->name)); break; case CVMX_HELPER_INTERFACE_MODE_LOOP: dev->netdev_ops = _oct_npi_netdev_ops; - strcpy(dev->name, "loop%d"); + strscpy(dev->name, "loop%d", sizeof(dev->name)); break; case CVMX_HELPER_INTERFACE_MODE_SGMII: priv->phy_mode = PHY_INTERFACE_MODE_SGMII; dev->netdev_ops = _oct_sgmii_netdev_ops; - strcpy(dev->name, "eth%d"); + strscpy(dev->name, "eth%d", sizeof(dev->name)); break; case CVMX_HELPER_INTERFACE_MODE_SPI: dev->netdev_ops = _oct_spi_netdev_ops; - strcpy(dev->name, "spi%d"); + strscpy(dev->name, "spi%d", sizeof(dev->name)); break; case CVMX_HELPER_INTERFACE_MODE_GMII: priv->phy_mode = PHY_INTERFACE_MODE_GMII; dev->netdev_ops = _oct_rgmii_netdev_ops; - strcpy(dev->name, "eth%d"); + strscpy(dev->name, "eth%d", sizeof(dev->name)); break; case CVMX_HELPER_INTERFACE_MODE_RGMII: dev->netdev_ops = _oct_rgmii_netdev_ops; - strcpy(dev->name, "eth%d"); + strscpy(dev->name, "eth%d", sizeof(dev->name)); cvm_set_rgmii_delay(priv, interface, port_index); break; -- 2.23.0
[PATCH] Staging: exfat: Avoid use of strcpy
Replaced strcpy with strscpy in exfat_core.c. Signed-off-by: Sandro Volery --- drivers/staging/exfat/exfat_core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/exfat/exfat_core.c b/drivers/staging/exfat/exfat_core.c index da8c58149c35..c71b145e8a24 100644 --- a/drivers/staging/exfat/exfat_core.c +++ b/drivers/staging/exfat/exfat_core.c @@ -2964,7 +2964,7 @@ s32 resolve_path(struct inode *inode, char *path, struct chain_t *p_dir, if (strlen(path) >= (MAX_NAME_LENGTH * MAX_CHARSET_SIZE)) return FFS_INVALIDPATH; - strcpy(name_buf, path); + strscpy(name_buf, path, sizeof(name_buf)); nls_cstring_to_uniname(sb, p_uniname, name_buf, ); if (lossy) -- 2.23.0
[PATCH v3] Staging: gasket: Use temporaries to reduce line length.
Using temporaries for gasket_page_table entries to remove scnprintf()
statements and reduce line length, as suggested by Joe Perches. Thanks!
Signed-off-by: Sandro Volery
---
v3: Fixed faulty copy/paste of function
v2: Attempt to fix
v1: Original patch
drivers/staging/gasket/apex_driver.c | 20 +---
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/gasket/apex_driver.c
b/drivers/staging/gasket/apex_driver.c
index 2973bb920a26..46199c8ca441 100644
--- a/drivers/staging/gasket/apex_driver.c
+++ b/drivers/staging/gasket/apex_driver.c
@@ -509,6 +509,8 @@ static ssize_t sysfs_show(struct device *device, struct
device_attribute *attr,
struct gasket_dev *gasket_dev;
struct gasket_sysfs_attribute *gasket_attr;
enum sysfs_attribute_type type;
+ struct gasket_page_table *gpt;
+ uint val;
gasket_dev = gasket_sysfs_get_device_data(device);
if (!gasket_dev) {
@@ -524,29 +526,25 @@ static ssize_t sysfs_show(struct device *device, struct
device_attribute *attr,
}
type = (enum sysfs_attribute_type)gasket_attr->data.attr_type;
+ gpt = gasket_dev->page_table[0];
switch (type) {
case ATTR_KERNEL_HIB_PAGE_TABLE_SIZE:
- ret = scnprintf(buf, PAGE_SIZE, "%u\n",
- gasket_page_table_num_entries(
- gasket_dev->page_table[0]));
+ val = gasket_page_table_num_entries(gpt);
break;
case ATTR_KERNEL_HIB_SIMPLE_PAGE_TABLE_SIZE:
- ret = scnprintf(buf, PAGE_SIZE, "%u\n",
- gasket_page_table_num_simple_entries(
- gasket_dev->page_table[0]));
+ val = gasket_page_table_num_simple_entries(gpt);
break;
case ATTR_KERNEL_HIB_NUM_ACTIVE_PAGES:
- ret = scnprintf(buf, PAGE_SIZE, "%u\n",
- gasket_page_table_num_active_pages(
- gasket_dev->page_table[0]));
+ val = gasket_page_table_num_active_pages(gpt);
break;
default:
dev_dbg(gasket_dev->dev, "Unknown attribute: %s\n",
attr->attr.name);
ret = 0;
- break;
+ goto exit;
}
-
+ ret = scnprintf(buf, PAGE_SIZE, "%u\n", val);
+exit:
gasket_sysfs_put_attr(device, gasket_attr);
gasket_sysfs_put_device_data(device, gasket_dev);
return ret;
--
2.23.0
Re: [PATCH v2] Staging: gasket: Use temporaries to reduce line length.
Wow... I checked, compiled and still sent the wrong thing again. I'm gonna have
to give this up soon if i can't get it right.
Sandro V
> On 10 Sep 2019, at 07:06, Sandro Volery wrote:
>
> Using temporaries for gasket_page_table entries to remove scnprintf()
> statements and reduce line length, as suggested by Joe Perches. Thanks!
>
> Signed-off-by: Sandro Volery
> ---
> drivers/staging/gasket/apex_driver.c | 20 +---
> 1 file changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/staging/gasket/apex_driver.c
> b/drivers/staging/gasket/apex_driver.c
> index 2973bb920a26..16ac4329d65f 100644
> --- a/drivers/staging/gasket/apex_driver.c
> +++ b/drivers/staging/gasket/apex_driver.c
> @@ -509,6 +509,8 @@ static ssize_t sysfs_show(struct device *device, struct
> device_attribute *attr,
>struct gasket_dev *gasket_dev;
>struct gasket_sysfs_attribute *gasket_attr;
>enum sysfs_attribute_type type;
> +struct gasket_page_table *gpt;
> +uint val;
>
>gasket_dev = gasket_sysfs_get_device_data(device);
>if (!gasket_dev) {
> @@ -524,29 +526,25 @@ static ssize_t sysfs_show(struct device *device, struct
> device_attribute *attr,
>}
>
>type = (enum sysfs_attribute_type)gasket_attr->data.attr_type;
> +gpt = gasket_dev->page_table[0];
>switch (type) {
>case ATTR_KERNEL_HIB_PAGE_TABLE_SIZE:
> -ret = scnprintf(buf, PAGE_SIZE, "%u\n",
> -gasket_page_table_num_entries(
> -gasket_dev->page_table[0]));
> +val = gasket_page_table_num_simple_entries(gpt);
>break;
>case ATTR_KERNEL_HIB_SIMPLE_PAGE_TABLE_SIZE:
> -ret = scnprintf(buf, PAGE_SIZE, "%u\n",
> -gasket_page_table_num_simple_entries(
> -gasket_dev->page_table[0]));
> +val = gasket_page_table_num_simple_entries(gpt);
>break;
>case ATTR_KERNEL_HIB_NUM_ACTIVE_PAGES:
> -ret = scnprintf(buf, PAGE_SIZE, "%u\n",
> -gasket_page_table_num_active_pages(
> -gasket_dev->page_table[0]));
> +val = gasket_page_table_num_active_pages(gpt);
>break;
>default:
>dev_dbg(gasket_dev->dev, "Unknown attribute: %s\n",
>attr->attr.name);
>ret = 0;
> -break;
> +goto exit;
>}
> -
> +ret = scnprintf(buf, PAGE_SIZE, "%u\n", val);
> +exit:
>gasket_sysfs_put_attr(device, gasket_attr);
>gasket_sysfs_put_device_data(device, gasket_dev);
>return ret;
> --
> 2.23.0
>
[PATCH v2] Staging: gasket: Use temporaries to reduce line length.
Using temporaries for gasket_page_table entries to remove scnprintf()
statements and reduce line length, as suggested by Joe Perches. Thanks!
Signed-off-by: Sandro Volery
---
drivers/staging/gasket/apex_driver.c | 20 +---
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/gasket/apex_driver.c
b/drivers/staging/gasket/apex_driver.c
index 2973bb920a26..16ac4329d65f 100644
--- a/drivers/staging/gasket/apex_driver.c
+++ b/drivers/staging/gasket/apex_driver.c
@@ -509,6 +509,8 @@ static ssize_t sysfs_show(struct device *device, struct
device_attribute *attr,
struct gasket_dev *gasket_dev;
struct gasket_sysfs_attribute *gasket_attr;
enum sysfs_attribute_type type;
+ struct gasket_page_table *gpt;
+ uint val;
gasket_dev = gasket_sysfs_get_device_data(device);
if (!gasket_dev) {
@@ -524,29 +526,25 @@ static ssize_t sysfs_show(struct device *device, struct
device_attribute *attr,
}
type = (enum sysfs_attribute_type)gasket_attr->data.attr_type;
+ gpt = gasket_dev->page_table[0];
switch (type) {
case ATTR_KERNEL_HIB_PAGE_TABLE_SIZE:
- ret = scnprintf(buf, PAGE_SIZE, "%u\n",
- gasket_page_table_num_entries(
- gasket_dev->page_table[0]));
+ val = gasket_page_table_num_simple_entries(gpt);
break;
case ATTR_KERNEL_HIB_SIMPLE_PAGE_TABLE_SIZE:
- ret = scnprintf(buf, PAGE_SIZE, "%u\n",
- gasket_page_table_num_simple_entries(
- gasket_dev->page_table[0]));
+ val = gasket_page_table_num_simple_entries(gpt);
break;
case ATTR_KERNEL_HIB_NUM_ACTIVE_PAGES:
- ret = scnprintf(buf, PAGE_SIZE, "%u\n",
- gasket_page_table_num_active_pages(
- gasket_dev->page_table[0]));
+ val = gasket_page_table_num_active_pages(gpt);
break;
default:
dev_dbg(gasket_dev->dev, "Unknown attribute: %s\n",
attr->attr.name);
ret = 0;
- break;
+ goto exit;
}
-
+ ret = scnprintf(buf, PAGE_SIZE, "%u\n", val);
+exit:
gasket_sysfs_put_attr(device, gasket_attr);
gasket_sysfs_put_device_data(device, gasket_dev);
return ret;
--
2.23.0
Re: [PATCH] Staging: gasket: Use temporaries to reduce line length.
> On 10 Sep 2019, at 00:30, Joe Perches wrote:
>
> On Mon, 2019-09-09 at 22:28 +0200, Sandro Volery wrote:
>> Using temporaries for gasket_page_table entries to remove scnprintf()
>> statements and reduce line length, as suggested by Joe Perches. Thanks!
>
> nak. Slow down. You broke the code.
>
> Please be _way_ more careful and verify for yourself
> the code you submit _before_ you submit it.
>
> compile/test/verify, twice if necessary.
>
Shoot. I'm sorry I'm just really trying to get into all this...
> You also should have cc'd me on this patch.
>
Will do! I'll submit v2 this afternoon.
>> diff --git a/drivers/staging/gasket/apex_driver.c
>> b/drivers/staging/gasket/apex_driver.c
> []
>> @@ -524,29 +526,25 @@ static ssize_t sysfs_show(struct device *device,
>> struct device_attribute *attr,
>>}
>>
>>type = (enum sysfs_attribute_type)gasket_attr->data.attr_type;
>> +gpt = gasket_dev->page_table[0];
>>switch (type) {
>>case ATTR_KERNEL_HIB_PAGE_TABLE_SIZE:
>> -ret = scnprintf(buf, PAGE_SIZE, "%u\n",
>> -gasket_page_table_num_entries(
>> -gasket_dev->page_table[0]));
>> +val = gasket_page_table_num_simple_entries(gpt);
>
> You likely duplicated this line via copy/paste.
> This should be:
>val = gasket_page_table_num_entries(gpt);
>
Thanks for being patient with me so far... I'd imagine others would've freaked
out at me by now :)
[PATCH] Staging: gasket: Use temporaries to reduce line length.
Using temporaries for gasket_page_table entries to remove scnprintf()
statements and reduce line length, as suggested by Joe Perches. Thanks!
Signed-off-by: Sandro Volery
---
drivers/staging/gasket/apex_driver.c | 20 +---
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/gasket/apex_driver.c
b/drivers/staging/gasket/apex_driver.c
index 2973bb920a26..16ac4329d65f 100644
--- a/drivers/staging/gasket/apex_driver.c
+++ b/drivers/staging/gasket/apex_driver.c
@@ -509,6 +509,8 @@ static ssize_t sysfs_show(struct device *device, struct
device_attribute *attr,
struct gasket_dev *gasket_dev;
struct gasket_sysfs_attribute *gasket_attr;
enum sysfs_attribute_type type;
+ struct gasket_page_table *gpt;
+ uint val;
gasket_dev = gasket_sysfs_get_device_data(device);
if (!gasket_dev) {
@@ -524,29 +526,25 @@ static ssize_t sysfs_show(struct device *device, struct
device_attribute *attr,
}
type = (enum sysfs_attribute_type)gasket_attr->data.attr_type;
+ gpt = gasket_dev->page_table[0];
switch (type) {
case ATTR_KERNEL_HIB_PAGE_TABLE_SIZE:
- ret = scnprintf(buf, PAGE_SIZE, "%u\n",
- gasket_page_table_num_entries(
- gasket_dev->page_table[0]));
+ val = gasket_page_table_num_simple_entries(gpt);
break;
case ATTR_KERNEL_HIB_SIMPLE_PAGE_TABLE_SIZE:
- ret = scnprintf(buf, PAGE_SIZE, "%u\n",
- gasket_page_table_num_simple_entries(
- gasket_dev->page_table[0]));
+ val = gasket_page_table_num_simple_entries(gpt);
break;
case ATTR_KERNEL_HIB_NUM_ACTIVE_PAGES:
- ret = scnprintf(buf, PAGE_SIZE, "%u\n",
- gasket_page_table_num_active_pages(
- gasket_dev->page_table[0]));
+ val = gasket_page_table_num_active_pages(gpt);
break;
default:
dev_dbg(gasket_dev->dev, "Unknown attribute: %s\n",
attr->attr.name);
ret = 0;
- break;
+ goto exit;
}
-
+ ret = scnprintf(buf, PAGE_SIZE, "%u\n", val);
+exit:
gasket_sysfs_put_attr(device, gasket_attr);
gasket_sysfs_put_device_data(device, gasket_dev);
return ret;
--
2.23.0
Re: [PATCH] Fixed parentheses malpractice in apex_driver.c
> On 9 Sep 2019, at 18:15, Joe Perches wrote: > > On Sat, 2019-09-07 at 18:12 +0200, Sandro Volery wrote: >> Alright, I'll do that when I get home tonight! > > I'm going to assume this is not an actual problem. > Oh yeah I'm sorry, I was all busy reading documentations about the Kernel and stuff ;) I got it to work, I actually just had my gitconfig name set as 'volery'. Thanks >> Thanks, >> Sandro V >> >>>> On 7 Sep 2019, at 18:08, Joe Perches wrote: >>> >>> On Sat, 2019-09-07 at 17:56 +0200, Sandro Volery wrote: >>>>>> On 7 Sep 2019, at 17:44, Joe Perches wrote: >>>>> >>>>> On Sat, 2019-09-07 at 17:34 +0200, Sandro Volery wrote: >>>>>> On patchwork I entered 'volery' as my username because I didn't know >>>>>> better, and now checkpatch always complains when I add 'signed-off-by' >>>>>> with my actual full name. >>>>> >>>>> How does checkpatch complain? >>>>> There is no connection between patchwork >>>>> and checkpatch. >>>> >>>> Checkpatch tells me that I haven't used 'volery' as >>>> my signed off name. >>> >>> Please send the both the patch and the actual checkpatch output >>> you get when running 'perl ./scripts/checkpatch.pl ' >>> >>> If this patch is a commit in your own local git tree: >>> >>> $ git format-patch -1 --stdout > tmp >>> $ perl ./scripts/checkpatch.pl --strict tmp >>> >>> and send tmp and the checkpatch output. >>> >>> >
Re: [PATCH] Fixed most indent issues in tty_io.c
> strongly encouraged to restrict their checkpatch cleanups to the > staging tree, since when such cleanup patches are considered welcome > very much depends on the kernel subsystem and the maintainers > involved.) Should I still send the ones I tried to submit in this thread again as separate patches? Since Greg said I should do it that way so I'm not sure if I should do that quick or just stick to staging. Thanks, Sandro V.
Re: [PATCH] Fixed most indent issues in tty_io.c
> On 8 Sep 2019, at 22:59, Theodore Y. Ts'o wrote: > Hi Ted, Thank you, for elaborating all this to me! I will try to limit my patches to the staging tree, until I feel confident enough to tackle a real coding issue in the kernel. Thanks, Sandro V
[PATCH] Staging: wlan-ng: parenthesis at end of line fix
Fixed open parenthesis at the end of the line on line 327. Signed-off-by: Sandro Volery --- drivers/staging/wlan-ng/cfg80211.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/staging/wlan-ng/cfg80211.c b/drivers/staging/wlan-ng/cfg80211.c index eee1998c4b18..0a4c7415f773 100644 --- a/drivers/staging/wlan-ng/cfg80211.c +++ b/drivers/staging/wlan-ng/cfg80211.c @@ -324,7 +324,8 @@ static int prism2_scan(struct wiphy *wiphy, (i < request->n_channels) && i < ARRAY_SIZE(prism2_channels); i++) msg1.channellist.data.data[i] = - ieee80211_frequency_to_channel( + ieee80211_frequency_to_channel + ( request->channels[i]->center_freq); msg1.channellist.data.len = request->n_channels; -- 2.23.0
Re: [PATCH] Fixed most indent issues in tty_io.c
Sandro V >> On 7 Sep 2019, at 22:04, Joe Perches wrote: >> >> On Sat, 2019-09-07 at 11:09 +0200, volery wrote: >> There were a lot of styling problems using space then tab or spaces >> instead of tabs in that file. Especially the entire function at line >> 2677. >> Also added a space before the : on line 2221. > > You do not have an appropriate subject line. > > This subject should probably be something like: > > [PATCH] tty: tty_io: Use normal indentation > > Please make changes only in drivers/staging until you are > really familiar with the linux kernel patch process. Yep, noticed that I had the subject line wrong on this one too after Dan told me about that today. I'll take that drivers/staging thing into account :) Thanks again Sandro
Re: [PATCH] Fixed most indent issues in tty_io.c
> On 7 Sep 2019, at 21:27, Joe Perches wrote: > > On Sat, 2019-09-07 at 18:42 +0100, Greg KH wrote: >>> On Sat, Sep 07, 2019 at 07:35:42PM +0200, Sandro Volery wrote: >>> >>>>>> On 7 Sep 2019, at 19:29, Greg KH wrote: >>>>> On Sat, Sep 07, 2019 at 07:23:59PM +0200, Sandro Volery wrote: >>>>> Dear Greg, >>>>> I am pretty sure the issue was, that I did too many things at once. >>>>> However, all the things I did are related to spaces / tabs, maybe that >>>>> still works? >>>> >>>> >>>> >>>> For some reason you sent this only to me, which is a bit rude to >>>> everyone else on the mailing list. I'll be glad to respond if you >>>> resend it to everyone. >>> >>> I'm sorry, newbie here. I thought it'd be better to not annoy everyone with >>> responses but learning things everyday I guess :) >> >> No problem, but you should also line-wrap your emails :) >> >>> I am pretty sure the issue with my patch was that there was too many >>> changes, however all of them were spaces and tabs related, so I think this >>> could be fine? >> >> As the bot said, break it out into "one patch per logical change", and >> "fix all whitespace issues" is not "one logical change". > > As long as git diff -w shows no difference and a compiled > object comparison before and after the change shows no > difference, I think it's fine. My thoughts, too. I didn't feel comfortable arguing as a newbie tho so I'll see what I can do once I get home. > > In fact, it avoid multiple commits where the only changes > are whitespace. > > >
Re: [PATCH] Fixed most indent issues in tty_io.c
Sandro V > On 7 Sep 2019, at 20:03, Greg KH wrote: > > On Sat, Sep 07, 2019 at 07:49:43PM +0200, Sandro Volery wrote: >> >> >> >> >>>> On 7 Sep 2019, at 19:42, Greg KH wrote: >>> >>> On Sat, Sep 07, 2019 at 07:35:42PM +0200, Sandro Volery wrote: >>>> >>>> >>>>>>>> On 7 Sep 2019, at 19:29, Greg KH wrote: >>>>>>> On Sat, Sep 07, 2019 at 07:23:59PM +0200, Sandro Volery wrote: >>>>>>> Dear Greg, >>>>>>> I am pretty sure the issue was, that I did too many things at once. >>>>>>> However, all the things I did are related to spaces / tabs, maybe that >>>>>>> still works? >>>>> >>>>> >>>>> >>>>> For some reason you sent this only to me, which is a bit rude to >>>>> everyone else on the mailing list. I'll be glad to respond if you >>>>> resend it to everyone. >>>> >>>> I'm sorry, newbie here. I thought it'd be better to not annoy everyone >>>> with responses but learning things everyday I guess :) >>> >>> No problem, but you should also line-wrap your emails :) >> >> I've just been told that before, I'll try to change >> those settings asap. >> >>> >>>> I am pretty sure the issue with my patch was that there was too many >>>> changes, however all of them were spaces and tabs related, so I think this >>>> could be fine? >>> >>> As the bot said, break it out into "one patch per logical change", and >>> "fix all whitespace issues" is not "one logical change". >> >> So a logical change would be if I make one patch >> to replace all spaces with tabs and a separate >> patch to add a space before the : ? > > Yes, that would be good. Make it a patch series please. Thanks! Any suggestion on how I should do the line wrapping when I want to send emails from my phone? Since I don't always have my laptop handy. Sandro V.
Re: [PATCH] Fixed most indent issues in tty_io.c
> On 7 Sep 2019, at 19:42, Greg KH wrote: > > On Sat, Sep 07, 2019 at 07:35:42PM +0200, Sandro Volery wrote: >> >> >>>>>> On 7 Sep 2019, at 19:29, Greg KH wrote: >>>>> On Sat, Sep 07, 2019 at 07:23:59PM +0200, Sandro Volery wrote: >>>>> Dear Greg, >>>>> I am pretty sure the issue was, that I did too many things at once. >>>>> However, all the things I did are related to spaces / tabs, maybe that >>>>> still works? >>> >>> >>> >>> For some reason you sent this only to me, which is a bit rude to >>> everyone else on the mailing list. I'll be glad to respond if you >>> resend it to everyone. >> >> I'm sorry, newbie here. I thought it'd be better to not annoy everyone with >> responses but learning things everyday I guess :) > > No problem, but you should also line-wrap your emails :) I've just been told that before, I'll try to change those settings asap. > >> I am pretty sure the issue with my patch was that there was too many >> changes, however all of them were spaces and tabs related, so I think this >> could be fine? > > As the bot said, break it out into "one patch per logical change", and > "fix all whitespace issues" is not "one logical change". So a logical change would be if I make one patch to replace all spaces with tabs and a separate patch to add a space before the : ? Thanks, Sandro V.
Re: [PATCH] Fixed most indent issues in tty_io.c
>>> On 7 Sep 2019, at 19:29, Greg KH wrote: >> On Sat, Sep 07, 2019 at 07:23:59PM +0200, Sandro Volery wrote: >> Dear Greg, >> I am pretty sure the issue was, that I did too many things at once. However, >> all the things I did are related to spaces / tabs, maybe that still works? > > > > For some reason you sent this only to me, which is a bit rude to > everyone else on the mailing list. I'll be glad to respond if you > resend it to everyone. I'm sorry, newbie here. I thought it'd be better to not annoy everyone with responses but learning things everyday I guess :) I am pretty sure the issue with my patch was that there was too many changes, however all of them were spaces and tabs related, so I think this could be fine? Sincerely, Sandro V
Re: [PATCH] Fixed parentheses malpractice in apex_driver.c
Alright, I'll do that when I get home tonight! Thanks, Sandro V > On 7 Sep 2019, at 18:08, Joe Perches wrote: > > On Sat, 2019-09-07 at 17:56 +0200, Sandro Volery wrote: >>>> On 7 Sep 2019, at 17:44, Joe Perches wrote: >>> >>> On Sat, 2019-09-07 at 17:34 +0200, Sandro Volery wrote: >>>> On patchwork I entered 'volery' as my username because I didn't know >>>> better, and now checkpatch always complains when I add 'signed-off-by' >>>> with my actual full name. >>> >>> How does checkpatch complain? >>> There is no connection between patchwork >>> and checkpatch. >> >> Checkpatch tells me that I haven't used 'volery' as >> my signed off name. > > Please send the both the patch and the actual checkpatch output > you get when running 'perl ./scripts/checkpatch.pl ' > > If this patch is a commit in your own local git tree: > > $ git format-patch -1 --stdout > tmp > $ perl ./scripts/checkpatch.pl --strict tmp > > and send tmp and the checkpatch output. > >
Re: [PATCH] Fixed parentheses malpractice in apex_driver.c
> On 7 Sep 2019, at 17:44, Joe Perches wrote: > > On Sat, 2019-09-07 at 17:34 +0200, Sandro Volery wrote: >> On patchwork I entered 'volery' as my username because I didn't know better, >> and now checkpatch always complains when I add 'signed-off-by' with my >> actual full name. > > How does checkpatch complain? > There is no connection between patchwork > and checkpatch. Checkpatch tells me that I haven't used 'volery' as my signed off name. > > And do please set your email client to use shorter line lengths > or remember to add returns. > > 72 or so is typical. > Alright! Using my phone right now as I am out of office but will do :)
Re: [PATCH] Fixed parentheses malpractice in apex_driver.c
> On 7 Sep 2019, at 16:52, Dan Carpenter wrote: > Alright, thanks! Some stupid other question: On patchwork I entered 'volery' as my username because I didn't know better, and now checkpatch always complains when I add 'signed-off-by' with my actual full name. How can I avoid that? Regards, Sandro V.
Re: [PATCH] Fixed parentheses malpractice in apex_driver.c
> On 7 Sep 2019, at 16:39, Dan Carpenter wrote: > > You need a subject prefix. It should be something like: > > [PATCH] Staging: gasket: Fix parentheses malpractice in apex_driver.c > Thanks for the reply! I'll try to do that better for my next patch. > Generally "Fix" is considered better style than "Fixed". We aren't > going to care about that in staging, but the patch prefix is mandatory > so you will need to redo it anyway and might as well fix that as well. > >> On Fri, Sep 06, 2019 at 08:38:01PM +0200, volery wrote: >> There were some parentheses at the end of lines, which I took care of. >> This is my first patch. > ^^ > Put this sort of comments after the --- cut off line > >> >> Signed-off-by: Sandro Volery >> --- > ^^^ > Put it here. It will be removed when we apply the patch so it won't > be recorded in the git log. > Alright :) >> drivers/staging/gasket/apex_driver.c | 9 ++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) > > Joe's comments are, of course, correct as well. > > regards, > dan carpenter > I'll take a look at Joe's suggestions too sometime tonight. I'd feel kinda bad tho if I just apply his work and send it in? - Sandro Volery
