* [PATCH] net/9p/trans_virtio.c: add a terminal char for mount tag @ 2018-08-01 7:44 piaojun 2018-08-01 8:11 ` Dominique Martinet 0 siblings, 1 reply; 6+ messages in thread From: piaojun @ 2018-08-01 7:44 UTC (permalink / raw) To: Dominique Martinet Cc: akpm, Eric Van Hensbergen, Ron Minnich, Latchesar Ionkov, Linux Kernel Mailing List, v9fs-developer, Greg Kurz 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; -- ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] net/9p/trans_virtio.c: add a terminal char for mount tag 2018-08-01 7:44 [PATCH] net/9p/trans_virtio.c: add a terminal char for mount tag piaojun @ 2018-08-01 8:11 ` Dominique Martinet 2018-08-01 8:24 ` piaojun 0 siblings, 1 reply; 6+ messages in thread From: Dominique Martinet @ 2018-08-01 8:11 UTC (permalink / raw) To: piaojun; +Cc: akpm, Linux Kernel Mailing List, v9fs-developer, Greg Kurz 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net/9p/trans_virtio.c: add a terminal char for mount tag 2018-08-01 8:11 ` Dominique Martinet @ 2018-08-01 8:24 ` piaojun 2018-08-01 10:52 ` Greg Kurz 0 siblings, 1 reply; 6+ messages in thread From: piaojun @ 2018-08-01 8:24 UTC (permalink / raw) To: Dominique Martinet Cc: akpm, Linux Kernel Mailing List, v9fs-developer, Greg Kurz 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; >> -- ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net/9p/trans_virtio.c: add a terminal char for mount tag 2018-08-01 8:24 ` piaojun @ 2018-08-01 10:52 ` Greg Kurz 2018-08-01 12:09 ` Dominique Martinet 0 siblings, 1 reply; 6+ messages in thread From: Greg Kurz @ 2018-08-01 10:52 UTC (permalink / raw) To: piaojun Cc: Dominique Martinet, akpm, Linux Kernel Mailing List, v9fs-developer 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; > >> -- ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net/9p/trans_virtio.c: add a terminal char for mount tag 2018-08-01 10:52 ` Greg Kurz @ 2018-08-01 12:09 ` Dominique Martinet 2018-08-02 1:18 ` piaojun 0 siblings, 1 reply; 6+ messages in thread From: Dominique Martinet @ 2018-08-01 12:09 UTC (permalink / raw) To: Greg Kurz; +Cc: piaojun, akpm, Linux Kernel Mailing List, v9fs-developer 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net/9p/trans_virtio.c: add a terminal char for mount tag 2018-08-01 12:09 ` Dominique Martinet @ 2018-08-02 1:18 ` piaojun 0 siblings, 0 replies; 6+ messages in thread From: piaojun @ 2018-08-02 1:18 UTC (permalink / raw) To: Dominique Martinet, Greg Kurz Cc: akpm, Linux Kernel Mailing List, v9fs-developer 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. > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-08-02 1:19 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-01 7:44 [PATCH] net/9p/trans_virtio.c: add a terminal char for mount tag piaojun 2018-08-01 8:11 ` Dominique Martinet 2018-08-01 8:24 ` piaojun 2018-08-01 10:52 ` Greg Kurz 2018-08-01 12:09 ` Dominique Martinet 2018-08-02 1:18 ` piaojun
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.