Re: [PATCH v3 3/5] ch: events: Read and parse cloud-hypervisor events

Thanks for the review, Michal. I will add your suggestion to not abort() and fix the possible memory leak issue in V4. On Mon, Nov 11, 2024 at 02:37:00PM +0100, Michal Prívozník wrote: > On 10/23/24 10:02, Purna Pavan Chandra Aekkaladevi wrote: > > Implement `chReadProcessEvents` and `chProcessEvents` to read events from > > event monitor FIFO file and parse them accordingly. > > > > diff --git a/src/ch/ch_events.c b/src/ch/ch_events.c > > index bb27f340e2..d0ad5af0af 100644 > > --- a/src/ch/ch_events.c > > +++ b/src/ch/ch_events.c > > @@ -28,6 +28,136 @@ > > > > +static void virCHReadProcessEvents(virCHMonitor *mon, > > + int event_monitor_fd) > > +{ > > + /* Event json string must always terminate with null char. > > + * So, reserve one byte for '\0' at the end. > > + */ > > + size_t max_sz = CH_EVENT_BUFFER_SZ - 1; > > + char *buf = mon->event_buffer.buffer; > > + virDomainObj *vm = mon->vm; > > + bool incomplete = false; > > + size_t sz = 0; > > + > > + memset(buf, 0, max_sz); > > + do { > > + ssize_t ret; > > + > > + ret = read(event_monitor_fd, buf + sz, max_sz - sz); > > You mean saferead()? Sure, will use saferead().

> > Also, I know you're opened this event_monitor_fd in blocking mode, so > this won't loop every second, so I'm not going to require use of event loop. > Yes, event_monitor_fd is in blocking mode and it might seem like this loop is not required. But we could end up read()-ing a partial event json; a loop here makes it possible to read the rest of the json ... > > + if (ret == 0 || (ret < 0 && errno == EINTR)) { > > + g_usleep(G_USEC_PER_SEC); > > + continue; > > + } else if (ret < 0) { > > + /* We should never reach here. read(2) says possible errors > > + * are EINTR, EAGAIN, EBADF, EFAULT, EINVAL, EIO, EISDIR > > + * We handle EINTR gracefully. There is some serious issue > > + * if we encounter any of the other errors(either in our code > > + * or in the system). Better to bail out. > > + */ > > + VIR_ERROR(_("%1$s: Failed to read ch events!: %2$s"), > > + vm->def->name, g_strerror(errno)); > > + VIR_FORCE_CLOSE(event_monitor_fd); > > + abort(); > > Again, aborting is unacceptable. > > > + } > > + > > + sz += ret; > > + mon->event_buffer.buf_fill_sz = sz; > > + > > + if (virCHProcessEvents(mon) < 0) > > + VIR_WARN("%s: Failed to parse and process events", vm->def->name); > > + > > + if (mon->event_buffer.buf_fill_sz != 0) > > + incomplete = true; > > + else > > + incomplete = false; > > + sz = mon->event_buffer.buf_fill_sz; > > + > > + } while (virDomainObjIsActive(vm) && (sz < max_sz) && incomplete); > > + ... and the loop is run only when the read() event json is incomplete. > > + return; > > +} > > + > > Michal Regards, Pavan