net/9p/trans_virtio.c: add a terminal char for mount tag
PATCH 1/1 net/9p/trans_virtio.c: add a terminal char for mount tag
chan->tag has no terminal char at last which will result in printing messy code when debugging code. So we should add '\0' for tag.
--- net/9p/trans_virtio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
net/9p/trans_virtio.c +1 -1
@@ -585,7 +585,7 @@ static int p9_virtio_probe(struct virtio_device *vdev) err = -EINVAL ; goto out_free_vq; } - tag =kmalloc (tag_len,GFP_KERNEL ); + tag =kzalloc (tag_len +1 ,GFP_KERNEL );if (!tag) { err = -ENOMEM ; goto out_free_vq;
Dominique Martinet
maintainerpiaojun wrote on Wed, Aug 01, 2018:
Quoted 2 lines
chan->taghas no terminal char at last which will result in printing messy code when debugging code. So we should add '\0' for tag.
9p is full of non null-terminated string so I'm not sure how I feel about it, is there anything wrong with how this is used or was this just when you tried to printf it?
If it's just for debugging I'd suggest using the printf format "%.*s" with "chan->tag_len, chan->tag" arguments,
That said it's not like this is costly, so I'll take it if someone else thinks this is helpful
Quoted 20 lines
Signed-off-by Jun Piao <[email protected]>--- net/9p/trans_virtio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)net/9p/trans_virtio.c +1
-1@@ -585,7 +585,7 @@ static int p9_virtio_probe(struct virtio_device *vdev) err = -EINVAL ; goto out_free_vq; } - tag =kmalloc (tag_len,GFP_KERNEL ); + tag =kzalloc (tag_len +1 ,GFP_KERNEL );if (!tag) { err = -ENOMEM ; goto out_free_vq;
Signature · 1 line
Dominique
Hi Dominique,
Quoted 7 lines · Dominique Martinet
piaojun wrote on Wed, Aug 01, 2018:
Quoted 2 lines
chan->taghas no terminal char at last which will result in printing messy code when debugging code. So we should add '\0' for tag.9p is full of non null-terminated string so I'm not sure how I feel about it, is there anything wrong with how this is used or was this just when you tried to printf it?
There is nothing wrong at the places using tag, as they calculated the tag_len carefully. Adding '\0' for it will make the code more robust. And I'm glad to hear others' opinions.
Thanks,
Jun
Quoted 28 lines
If it's just for debugging I'd suggest using the printf format "%.*s" with "
chan->tag_len,chan->tag" arguments,That said it's not like this is costly, so I'll take it if someone else thinks this is helpful
Quoted 20 lines
Signed-off-by Jun Piao <[email protected]>--- net/9p/trans_virtio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)net/9p/trans_virtio.c +1
-1@@ -585,7 +585,7 @@ static int p9_virtio_probe(struct virtio_device *vdev) err = -EINVAL ; goto out_free_vq; } - tag =kmalloc (tag_len,GFP_KERNEL ); + tag =kzalloc (tag_len +1 ,GFP_KERNEL );if (!tag) { err = -ENOMEM ; goto out_free_vq;
Quoted 11 lines · On Wed, 1 Aug 2018 16:24:27 +0800 piaojun <[email protected]> wrote:
Hi Dominique,
Quoted 7 lines · Dominique Martinet
piaojun wrote on Wed, Aug 01, 2018:
Quoted 2 lines
chan->taghas no terminal char at last which will result in printing messy code when debugging code. So we should add '\0' for tag.9p is full of non null-terminated string so I'm not sure how I feel about it, is there anything wrong with how this is used or was this just when you tried to printf it?
The mount tag isn't a 9P thingy actually. It is provided by the server in the device config space, and it is never used in any 9P message. It could have been a nul terminated string from the beginning. Not sure why people opted to store it that way.
Quoted 4 lines
There is nothing wrong at the places using tag, as they calculated the
tag_lencarefully. Adding '\0' for it will make the code more robust. And I'm glad to hear others' opinions.
So this patch basically turns chan->tag into a nul terminated string, which is indeed more convenient and robust. Maybe you can update the rest of the code accordingly and drop chan->tag_len then ?
Quoted 6 lines
Thanks,
JunQuoted 3 lines
If it's just for debugging I'd suggest using the printf format "%.*s" with "
chan->tag_len,chan->tag" arguments,
FWIW, 9P strings received from the client are also converted to nul terminated strings:
static int
p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt,
va_list ap)
{
[...]
case 's':{
[...]
*sptr = kmalloc(len + 1, GFP_NOFS);
[...]
(*sptr)[len] = 0;
}Cheers,
--
Greg
Quoted 25 lines
Quoted 25 lines
That said it's not like this is costly, so I'll take it if someone else thinks this is helpful
Quoted 20 lines
Signed-off-by Jun Piao <[email protected]>--- net/9p/trans_virtio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)net/9p/trans_virtio.c +1
-1@@ -585,7 +585,7 @@ static int p9_virtio_probe(struct virtio_device *vdev) err = -EINVAL ; goto out_free_vq; } - tag =kmalloc (tag_len,GFP_KERNEL ); + tag =kzalloc (tag_len +1 ,GFP_KERNEL );if (!tag) { err = -ENOMEM ; goto out_free_vq;
Dominique Martinet
maintainerGreg Kurz wrote on Wed, Aug 01, 2018:
Quoted 3 lines
So this patch basically turns
chan->taginto a nul terminated string, which is indeed more convenient and robust. Maybe you can update the rest of the code accordingly and dropchan->tag_lenthen ?
If we can use that to simplify some other part of the code, that's certainly more appealing to me :)
Quoted 2 lines
FWIW, 9P strings received from the client are also converted to nul terminated strings:
Oh, good to know; I guess that makes sense when these strings come and go from/to other components of the kernel that likely expect that.
Signature · 1 line
Dominique
Hi Dominique and Greg,
Thanks for your reviewing, and I will try to simplify other related code according your suggestions in patch v2.
Thanks,
Jun
Quoted 15 lines · Dominique Martinet
Greg Kurz wrote on Wed, Aug 01, 2018:
Quoted 3 lines
So this patch basically turns
chan->taginto a nul terminated string, which is indeed more convenient and robust. Maybe you can update the rest of the code accordingly and dropchan->tag_lenthen ?If we can use that to simplify some other part of the code, that's certainly more appealing to me :)
Quoted 2 lines
FWIW, 9P strings received from the client are also converted to nul terminated strings:
Oh, good to know; I guess that makes sense when these strings come and go from/to other components of the kernel that likely expect that.