Re: [PATCH v3 7/7] tools/proctrace: add proctrace tool

[Ian Jackson <ian.jackson@citrix.com>]

Tamas K Lengyel writes ("Re: [PATCH v3 7/7] tools/proctrace: add proctrace tool"):
> On Fri, Jun 26, 2020 at 7:26 AM Ian Jackson <ian.jackson@citrix.com> wrote:
> > Wei Liu writes ("Re: [PATCH v3 7/7] tools/proctrace: add proctrace tool"):
> > > On Mon, Jun 22, 2020 at 08:12:56PM +0200, Michał Leszczyński wrote:
> > > > Add an demonstration tool that uses xc_vmtrace_* calls in order
> > > > to manage external IPT monitoring for DomU.
> > ...
> > > > +    if (rc) {
> > > > +        fprintf(stderr, "Failed to call xc_vmtrace_pt_disable\n");
> > > > +        return 1;
> > > > +    }
> > > > +
> > >
> > > You should close fmem and xc in the exit path.
> >
> > Thanks for reviewing this.  I agree with your comments.  But I
> > disagree with this one.
> >
> > This is in main().  When the program exits, the xc handle and memory
> > mappings will go away as the kernel tears down the process.
> >
> > Leaving out this kind of cleanup in standalone command-line programs
> > is fine, I think.  It can make the code simpler - and it avoids the
> > risk of doing it wrong, which can turn errors into crashes.
> 
> Hi Ian,
> while I agree that this particular code would not be an issue,
> consider that these tools are often taken as sample codes to be reused
> in other contexts as well. As such, I think it should include the
> close bits as well and exhibit all the "best practices" we would like
> to see anyone else building tools for Xen.

Well, you're the author if this and I think you get to decide this
question (which is one of style).  If that is your view then you Wei's
comment is certainly right, as far as it goes.

But looking at this program it seems to me that there is a great deal
of other stuff it allocates, one way or another, which it doesn't
free.

Is your intent that this program has this coding style ?

   wombat = xc_allocate_wombat();
   if (bad(wombat)) {
     print_error("wombat");
     exit(-1);
   }

   hippo = xc_allocate_hippo();
   if (bad(hippo)) {
     print_error("hippo");
     xc_free_wombat(wombat);
     exit(-1);
   }

   zebra = xc_allocate_zebra();
   if (bad(zebra)) {
     print_error("zebra");
     xc_free_wombat(wombat);
     xc_free_hippo(hippo);
     exit(-1);
   }
   ...

I think this is an unhelpful coding style.  It inevitably leads to
leaks.  IMO if you are going to try to tear down all things, you
should do it like this:

   xc_wombat *wombat = NULL:
   xc_hippo *hippo = NULL;
   xc_hippo *zebra = NULL;

   wombat = xc_allocate_wombat();
   if (bad(wombat)) {
     print_error("wombat");
     goto exit_error;
   }

   hippo = xc_allocate_hippo();
   if (bad(hipp)) {
     print_error("hippo");
     goto exit_error;
   }

   zebra = xc_allocate_zebra();
   if (bad(hipp)) {
     print_error("zebra");
     goto exit_error;
   }

   ...
  exit_error:
   if (some(wombat)) xc_free_wombat(wombat);
   if (some(hippo)) xc_free_hippo(hippo);
   if (some(zebra)) xc_free_zebra(zebra);
   exit(-1);

or some similar approach that makes it easier to see that the code is
correct and leak-free.

But as I say I think as the author you get to decide.

Regards,
Ian.