Re: [PATCH 1/4] virsh: Refactor event printing

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Dec 21, 2015 at 14:46:33 +0100, Andrea Bolognani wrote:
> n Mon, 2015-12-21 at 11:04 +0100, Jiri Denemark wrote:
> > To reduce code duplication.
> > 
> > Signed-off-by: Jiri Denemark <jdenemar@xxxxxxxxxx>
> > ---
> >  tools/virsh-domain.c | 293 ++++++++++++++++++++++++---------------------------
> >  1 file changed, 136 insertions(+), 157 deletions(-)
> 
> Really nice cleanup, looks good overall.
> 
> A couple of comments below.
> 
> >  static void
> > -virshEventPrint(virConnectPtr conn ATTRIBUTE_UNUSED, virDomainPtr dom,
> > -                const char *event, long long seconds, unsigned int micros,
> > -                const char *details, void *opaque)
> > +virshEventQemuPrint(virConnectPtr conn ATTRIBUTE_UNUSED,
> > +                    virDomainPtr dom,
> > +                    const char *event,
> > +                    long long seconds,
> > +                    unsigned int micros,
> > +                    const char *details,
> > +                    void *opaque)
> >  {
> >      virshQemuEventData *data = opaque;
> >      virJSONValuePtr pretty = NULL;
> 
> A comment describing the functions would be nice. Their use is
> pretty obvious, so not a deal breaker.

Done. Is it a deal? :-)

> >  static void
> > +virshEventPrint(virshDomEventData *data,
> > +                virBufferPtr buf)
> > +{
> > +    char *msg;
> > +
> > +    if (!(msg = virBufferContentAndReset(buf)))
> > +        return;
> > +
> > +    if (!data->loop && *data->count)
> > +        goto cleanup;
> > +
> > +    vshPrint(data->ctl, "%s", msg);
> > +
> > +    (*data->count)++;
> > +    if (!data->loop)
> > +        vshEventDone(data->ctl);
> > +
> > + cleanup:
> > +    VIR_FREE(msg);
> > +}
> 
> This works just fine, however I dislike the fact that the virBuffer
> is allocated by the caller but released here.
> 
> I'd rather use virBufferCurrentContent() here and leave the caller
> responsible for releasing the buffer. Doing so would make the callers
> slightly more verbose, but result in cleaner code overall IMHO.

I think we just need a comment explicitly saying that this function will
free the buffer. And I added it to the description requested by your
not-a-deal-breaker comment.

Is this good enough?

    +/**
    + * virshEventPrint:
    + *
    + * @data: opaque data passed to all event callbacks
    + * @buf: string buffer describing the event
    + *
    + * Print the event description found in @buf and update virshDomEventData.
    + *
    + * This function resets @buf and frees all memory consumed by its content.
    + */

> > @@ -12237,26 +12249,26 @@ virshEventGraphicsPrint(virConnectPtr conn ATTRIBUTE_UNUSED,
> >                          const virDomainEventGraphicsSubject *subject,
> >                          void *opaque)
> >  {
> > -    virshDomEventData *data = opaque;
> > +    virBuffer buf = VIR_BUFFER_INITIALIZER;
> >      size_t i;
> >  
> > -    if (!data->loop && *data->count)
> > -        return;
> > -    vshPrint(data->ctl, _("event 'graphics' for domain %s: "
> > -                          "%s local[%s %s %s] remote[%s %s %s] %s"),
> > -             virDomainGetName(dom), virshGraphicsPhaseToString(phase),
> > -             virshGraphicsAddressToString(local->family),
> > -             local->node, local->service,
> > -             virshGraphicsAddressToString(remote->family),
> > -             remote->node, remote->service,
> > -             authScheme);
> > -    for (i = 0; i < subject->nidentity; i++)
> > -        vshPrint(data->ctl, " %s=%s", subject->identities[i].type,
> > -                 subject->identities[i].name);
> > -    vshPrint(data->ctl, "\n");
> > -    (*data->count)++;
> > -    if (!data->loop)
> > -        vshEventDone(data->ctl);
> > +    virBufferAsprintf(&buf, _("event 'graphics' for domain %s: "
> > +                              "%s local[%s %s %s] remote[%s %s %s] %s\n"),
> > +                      virDomainGetName(dom),
> > +                      virshGraphicsPhaseToString(phase),
> > +                      virshGraphicsAddressToString(local->family),
> > +                      local->node,
> > +                      local->service,
> > +                      virshGraphicsAddressToString(remote->family),
> > +                      remote->node,
> > +                      remote->service,
> > +                      authScheme);
> > +    for (i = 0; i < subject->nidentity; i++) {
> > +        virBufferAsprintf(&buf, "\t%s=%s\n",
> > +                          subject->identities[i].type,
> > +                          subject->identities[i].name);
> > +    }
> > +    virshEventPrint(opaque, &buf);
> >  }
> 
> You're changing the format a bit here - I like the new one better, and
> it's the same used in virshEventTunablePrint() below. I'm just concerned
> about users parsing this in some way and being confused by the change.

Right, I changed the format to be consistent with another event which
also reported name=value stuff. I don't think we need to be worried
about the output in this particular case.

Jirka

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]