[PATCH] net/9p/trans_virtio.c: add a terminal char for mount tag

[PATCH] net/9p/trans_virtio.c: add a terminal char for mount tag

[piaojun]

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.

Signed-off-by: Jun Piao <piaojun@huawei.com>
---
 net/9p/trans_virtio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
index d422bfc..49d71d6 100644
--- a/net/9p/trans_virtio.c
+++ b/net/9p/trans_virtio.c
@@ -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;
-- 

Re: [PATCH] net/9p/trans_virtio.c: add a terminal char for mount tag

[Dominique Martinet]

piaojun wrote on Wed, Aug 01, 2018:
> 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.

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

> 
> Signed-off-by: Jun Piao <piaojun@huawei.com>
> ---
>  net/9p/trans_virtio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
> index d422bfc..49d71d6 100644
> --- a/net/9p/trans_virtio.c
> +++ b/net/9p/trans_virtio.c
> @@ -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 

Re: [PATCH] net/9p/trans_virtio.c: add a terminal char for mount tag

[piaojun]

Hi Dominique,

On 2018/8/1 16:11, Dominique Martinet wrote:
> piaojun wrote on Wed, Aug 01, 2018:
>> 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.
> 
> 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

> 
> 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
> 
>>
>> Signed-off-by: Jun Piao <piaojun@huawei.com>
>> ---
>>  net/9p/trans_virtio.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
>> index d422bfc..49d71d6 100644
>> --- a/net/9p/trans_virtio.c
>> +++ b/net/9p/trans_virtio.c
>> @@ -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;
>> -- 

Re: [PATCH] net/9p/trans_virtio.c: add a terminal char for mount tag

[Greg Kurz]

On Wed, 1 Aug 2018 16:24:27 +0800
piaojun <piaojun@huawei.com> wrote:

> Hi Dominique,
> 
> On 2018/8/1 16:11, Dominique Martinet wrote:
> > piaojun wrote on Wed, Aug 01, 2018:  
> >> 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.  
> > 
> > 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.

> 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.
> 

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 ?

> Thanks,
> Jun
> 
> > 
> > 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

> > 
> > 
> > That said it's not like this is costly, so I'll take it if someone else
> > thinks this is helpful
> >   
> >>
> >> Signed-off-by: Jun Piao <piaojun@huawei.com>
> >> ---
> >>  net/9p/trans_virtio.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
> >> index d422bfc..49d71d6 100644
> >> --- a/net/9p/trans_virtio.c
> >> +++ b/net/9p/trans_virtio.c
> >> @@ -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;
> >> --   

Re: [PATCH] net/9p/trans_virtio.c: add a terminal char for mount tag

[Dominique Martinet]

Greg Kurz wrote on Wed, Aug 01, 2018:
> 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 ?

If we can use that to simplify some other part of the code, that's
certainly more appealing to me :)


> 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.

-- 
Dominique

Re: [PATCH] net/9p/trans_virtio.c: add a terminal char for mount tag

[piaojun]

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

On 2018/8/1 20:09, Dominique Martinet wrote:
> Greg Kurz wrote on Wed, Aug 01, 2018:
>> 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 ?
> 
> If we can use that to simplify some other part of the code, that's
> certainly more appealing to me :)
> 
> 
>> 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.
>