[Libguestfs] [PATCH v3 1/2] New tool: virt-tail.

[Richard W.M. Jones]

On Tue, Oct 11, 2016 at 11:38:35AM +0200, Pino Toscano wrote:
> On Monday, 3 October 2016 15:07:10 CEST Richard W.M. Jones wrote:
> > This follows (tails) a log file within a guest, rather like
> > the regular 'tail -f' command.  For example:
> > 
> >   virt-tail -d guest /var/log/messages
> > ---
> 
> Mostly LGTM, just a few notes.
> 
> > +      guestfs_push_error_handler (g, NULL, NULL);
> > +      stat = guestfs_statns (g, filename);
> > +      guestfs_pop_error_handler (g);
> > +      if (stat == NULL) {
> > +        /* There's an error.  Treat ENOENT as if the file was empty size. */
> > +        if (guestfs_last_errno (g) == ENOENT) {
> > +          time (&t);
> > +          file[i].mtime = t;
> > +          file[i].size = 0;
> 
> I'd set size as -1, otherwise this is considering the file as existing.
> If virt-tail (wants to) behaves like `tail -f`, I'd:
> - print an error/message line (not fatal) to warn that a file does not
>   exist
> - when it appears, print that and start following it

As the comment says, it's supposed to consider the file as if it
exists.  Note there is code later on which considers the case that all
files have been deleted (`if (processed == 0) ...').

> > +          /* If we get here, the file changed and we're going to display
> > +           * something.  If there is more than one file, and the file
> > +           * displayed is different from previously, then display the
> > +           * filename banner.
> > +           */
> > +          if (i != prev_file_displayed)
> > +            printf ("\n\n--- %s ---\n\n", filename);
> > +          prev_file_displayed = i;
> 
> I'd simplify the check as "if (argc > 1)", and remove the
> "prev_file_displayed" variable (not used otherwise).

I don't understand.  The code is supposed to print the banner if the
file we are displaying is different from the previous file we are
displaying, ie. if switching between one of several files.  AFAICT
`if (argc > 1)' doesn't do that.

> > +    /* Do nothing until something happens on the disk image.  Even if
> > +     * the drive changes, always wait min. 30 seconds.  For libvirt
> > +     * (-d) and remote sources we cannot check this so we have to use
> > +     * a fixed (5 minute) delay instead.  Also we recheck every so
> > +     * often even if nothing seems to have changed.  (XXX Can we do
> > +     * better?)
> 
> Should -s/--sleep-interval be available too, then?

Yup, room for a future enhancement.  However I wasn't sure what the
best thing to do here is.  Adding a --sleep option ties us to a
particular implementation now.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html