[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PULL 16/17] COLO-compare: Add colo-compare remote noti
From: |
Zhang, Chen |
Subject: |
Re: [Qemu-devel] [PULL 16/17] COLO-compare: Add colo-compare remote notify support |
Date: |
Wed, 3 Jul 2019 01:20:21 +0000 |
> -----Original Message-----
> From: Zhang, Chen
> Sent: Wednesday, July 3, 2019 9:08 AM
> To: 'Peter Maydell' <address@hidden>; Jason Wang
> <address@hidden>
> Cc: QEMU Developers <address@hidden>
> Subject: RE: [PULL 16/17] COLO-compare: Add colo-compare remote notify
> support
>
>
>
> > -----Original Message-----
> > From: Peter Maydell [mailto:address@hidden]
> > Sent: Wednesday, July 3, 2019 2:51 AM
> > To: Jason Wang <address@hidden>
> > Cc: QEMU Developers <address@hidden>; Zhang, Chen
> > <address@hidden>
> > Subject: Re: [PULL 16/17] COLO-compare: Add colo-compare remote notify
> > support
> >
> > On Tue, 2 Jul 2019 at 03:32, Jason Wang <address@hidden> wrote:
> > >
> > > From: Zhang Chen <address@hidden>
> > >
> > > This patch make colo-compare can send message to remote COLO
> > > frame(Xen)
> > when occur checkpoint.
> > >
> > > Signed-off-by: Zhang Chen <address@hidden>
> > > Signed-off-by: Jason Wang <address@hidden>
> >
> > Hi; Coverity reports a problem (CID 1402785) with this function:
> >
> > > @@ -989,7 +1006,24 @@ static void
> > > compare_sec_rs_finalize(SocketReadState *sec_rs)
> > >
> > > static void compare_notify_rs_finalize(SocketReadState *notify_rs)
> > > {
> > > + CompareState *s = container_of(notify_rs, CompareState,
> > > + notify_rs);
> > > +
> > > /* Get Xen colo-frame's notify and handle the message */
> > > + char *data = g_memdup(notify_rs->buf, notify_rs->packet_len);
> > > + char msg[] = "COLO_COMPARE_GET_XEN_INIT";
> > > + int ret;
> > > +
> > > + if (!strcmp(data, "COLO_USERSPACE_PROXY_INIT")) {
> > > + ret = compare_chr_send(s, (uint8_t *)msg, strlen(msg), 0, true);
> > > + if (ret < 0) {
> > > + error_report("Notify Xen COLO-frame INIT failed");
> > > + }
> > > + }
> > > +
> > > + if (!strcmp(data, "COLO_CHECKPOINT")) {
> > > + /* colo-compare do checkpoint, flush pri packet and remove
> > > + sec packet
> > */
> > > + g_queue_foreach(&s->conn_list, colo_flush_packets, s);
> > > + }
> > > }
> >
> > We allocate data using g_memdup(), but we never free it before
> > returning, so the function has a memory leak. It's not clear to me why
> > we're duplicating the string at all -- it would be cleaner to do the
> > check of the packet contents to identify in-place. That would be the best
> > way
> to fix/avoid the leak.
> >
> > Some other things I notice reading the function:
> >
> > (1) after the first if() we will go ahead and check the second if().
> > This means we'll unnecessarily check whether the data string matches
> > COLO_CHECKPOINT, when we know already it cannot. I think an if
> > (!strcmp(...)) { ... } else if (!strcmp(...)) { ... } structure would be
> > more normal
> C here.
> >
> > (2) the g_memdup() call is treating the data as a buffer-and-length,
> > and we don't enforce that it is NUL-terminated. But then we do a
> > strcmp() against it, which assumes that the data is a NUL-terminated
> > string. Is this safe ?
> >
> > (3) More minor point: you could mark 'msg' as const here, since I
> > think we never need to modify it.
>
> OK, I will fix it in next version.
> Thank you for your review.
>
I found this patch has been merged, I will send a patch to fix the issues of
peter comments.
Thanks
Zhang Chen
>
> Thanks
> Zhang Chen
>
> >
> > thanks
> > -- PMM
- [Qemu-devel] [PULL 05/17] net: avoid using variable length array in net_client_init(), (continued)
- [Qemu-devel] [PULL 05/17] net: avoid using variable length array in net_client_init(), Jason Wang, 2019/07/01
- [Qemu-devel] [PULL 10/17] net/announce: Add optional ID, Jason Wang, 2019/07/01
- [Qemu-devel] [PULL 11/17] net/announce: Add HMP optional ID, Jason Wang, 2019/07/01
- [Qemu-devel] [PULL 12/17] net/announce: Expand test for stopping self announce, Jason Wang, 2019/07/01
- [Qemu-devel] [PULL 13/17] COLO-compare: Add new parameter to communicate with remote colo-frame, Jason Wang, 2019/07/01
- [Qemu-devel] [PULL 14/17] COLO-compare: Add remote notification chardev handler frame, Jason Wang, 2019/07/01
- [Qemu-devel] [PULL 15/17] COLO-compare: Make the compare_chr_send() can send notification message., Jason Wang, 2019/07/01
- [Qemu-devel] [PULL 16/17] COLO-compare: Add colo-compare remote notify support, Jason Wang, 2019/07/01
- [Qemu-devel] [PULL 17/17] migration/colo.c: Add missed filter notify for Xen COLO., Jason Wang, 2019/07/01
- Re: [Qemu-devel] [PULL 00/17] Net patches, Peter Maydell, 2019/07/02