tty: prefer zalloc to malloc for inverted_path allocation

Submitted by Cyrill Gorcunov on June 23, 2016, 11:12 a.m.

Details

Message ID 20160623111216.GI2045@uranus.lan
State Rejected
Series "tty: prefer zalloc to malloc for inverted_path allocation"
Headers show

Commit Message

Cyrill Gorcunov June 23, 2016, 11:12 a.m.
On Thu, Jun 23, 2016 at 02:00:12PM +0300, Dmitry Safonov wrote:
> Otherwise, we copy original name to slash and after it there may stay
> some junk, which strcat will use for concatenation:
> (00.024843)     26: Error (files-reg.c:1528): Can't open file dev/pts/g:��ptmx on restore: No such file or directory
> (00.024846)     26: Error (files-reg.c:1470): Can't open file dev/pts/g:��ptmx: No such file or directory
> (00.024849)     26: Error (tty.c:545): tty: Can't open dev/pts/g:��ptmx: No such file or directory
> 
> Cc: Cyrill Gorcunov <gorcunov@openvz.org>
> Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com>

That's big hammer, won't the below oneliner fix it?
---

Patch hide | download patch | download mbox

diff --git a/criu/tty.c b/criu/tty.c
index 328fafe..0a535a9 100644
--- a/criu/tty.c
+++ b/criu/tty.c
@@ -477,6 +477,7 @@  static struct reg_file_info *pty_alloc_fake_reg(struct tty_info *info, int subty
 
                memcpy(inverted_path, orig->rfe->name, slash_at + 1);
                if (subtype == TTY_SUBTYPE_MASTER)
+                       inverted_path[slash_at + 1] = '\0';
                        strcat(inverted_path, "ptmx");
                else {
                        if (slash_at >= 3 && strncmp(&inverted_path[slash_at - 3], "pts", 3))

Comments

Dmitry Safonov June 23, 2016, 11:15 a.m.
On 06/23/2016 02:12 PM, Cyrill Gorcunov wrote:
> On Thu, Jun 23, 2016 at 02:00:12PM +0300, Dmitry Safonov wrote:
>> Otherwise, we copy original name to slash and after it there may stay
>> some junk, which strcat will use for concatenation:
>> (00.024843)     26: Error (files-reg.c:1528): Can't open file dev/pts/g:��ptmx on restore: No such file or directory
>> (00.024846)     26: Error (files-reg.c:1470): Can't open file dev/pts/g:��ptmx: No such file or directory
>> (00.024849)     26: Error (tty.c:545): tty: Can't open dev/pts/g:��ptmx: No such file or directory
>>
>> Cc: Cyrill Gorcunov <gorcunov@openvz.org>
>> Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com>
>
> That's big hammer, won't the below oneliner fix it?

Well, I thought about it -- I don't mind either version.
I think memsetting 32 bytes with zero in allocator or computing
explicitely here address + setting it to zero is quite the
same performance impact. So I guess, less code is better.
But, anyway, I don't mind against your version.

> ---
> diff --git a/criu/tty.c b/criu/tty.c
> index 328fafe..0a535a9 100644
> --- a/criu/tty.c
> +++ b/criu/tty.c
> @@ -477,6 +477,7 @@ static struct reg_file_info *pty_alloc_fake_reg(struct tty_info *info, int subty
>
>                 memcpy(inverted_path, orig->rfe->name, slash_at + 1);
>                 if (subtype == TTY_SUBTYPE_MASTER)
> +                       inverted_path[slash_at + 1] = '\0';
>                         strcat(inverted_path, "ptmx");
>                 else {
>                         if (slash_at >= 3 && strncmp(&inverted_path[slash_at - 3], "pts", 3))
>
Cyrill Gorcunov June 23, 2016, 11:21 a.m.
On Thu, Jun 23, 2016 at 02:15:24PM +0300, Dmitry Safonov wrote:
> On 06/23/2016 02:12 PM, Cyrill Gorcunov wrote:
> > On Thu, Jun 23, 2016 at 02:00:12PM +0300, Dmitry Safonov wrote:
> > > Otherwise, we copy original name to slash and after it there may stay
> > > some junk, which strcat will use for concatenation:
> > > (00.024843)     26: Error (files-reg.c:1528): Can't open file dev/pts/g:��ptmx on restore: No such file or directory
> > > (00.024846)     26: Error (files-reg.c:1470): Can't open file dev/pts/g:��ptmx: No such file or directory
> > > (00.024849)     26: Error (tty.c:545): tty: Can't open dev/pts/g:��ptmx: No such file or directory
> > > 
> > > Cc: Cyrill Gorcunov <gorcunov@openvz.org>
> > > Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com>
> > 
> > That's big hammer, won't the below oneliner fix it?
> 
> Well, I thought about it -- I don't mind either version.
> I think memsetting 32 bytes with zero in allocator or computing
> explicitely here address + setting it to zero is quite the
> same performance impact. So I guess, less code is better.
> But, anyway, I don't mind against your version.

zero allocations make sense when you know the data remains
zero initially (or you would setup it to zero manually),
but in our case we're allocating data and fill it immediately
with a path, so that it get written twice. The memory chunk
is small so there won't be performcance penalty seen.

Test it please ;) I didn't run real test on my version
but I guess you've the case under your hand.