Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Ian Jackson <ian.jackson@citrix.com>
To: Tamas K Lengyel <tamas.k.lengyel@gmail.com>
Cc: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	"Michał Leszczyński" <michal.leszczynski@cert.pl>,
	"Tamas K Lengyel" <tamas.lengyel@intel.com>,
	"Kang, Luwei" <luwei.kang@intel.com>, "Wei Liu" <wl@xen.org>
Subject: Re: [PATCH v3 7/7] tools/proctrace: add proctrace tool
Date: Wed, 1 Jul 2020 12:01:07 +0100
Message-ID: <24316.27891.433815.62003@mariner.uk.xensource.com> (raw)
In-Reply-To: <CABfawhmpGEE0jq=vMicqdmf2nbMs-a4Y0nxBUN=JwOeA_H-YGQ@mail.gmail.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.


  reply index

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-22 18:06 [PATCH v3 0/7] Implement support for external IPT monitoring Michał Leszczyński
2020-06-22 18:10 ` [PATCH v3 1/7] memory: batch processing in acquire_resource() Michał Leszczyński
2020-06-22 18:10 ` [PATCH v3 2/7] x86/vmx: add Intel PT MSR definitions Michał Leszczyński
2020-06-22 18:11 ` [PATCH v3 3/7] x86/vmx: add IPT cpu feature Michał Leszczyński
2020-06-22 18:11 ` [PATCH v3 4/7] x86/vmx: add do_vmtrace_op Michał Leszczyński
2020-06-23 11:54   ` Andrew Cooper
2020-06-29  9:52     ` Michał Leszczyński
2020-06-22 18:12 ` [PATCH v3 5/7] tools/libxc: add xc_vmtrace_* functions Michał Leszczyński
2020-06-26 11:50   ` Wei Liu
2020-06-22 18:12 ` [PATCH v3 6/7] tools/libxl: add vmtrace_pt_size parameter Michał Leszczyński
2020-06-26 11:52   ` Wei Liu
2020-06-22 18:12 ` [PATCH v3 7/7] tools/proctrace: add proctrace tool Michał Leszczyński
2020-06-23  9:35   ` Tamas K Lengyel
2020-06-26 11:48   ` Wei Liu
2020-06-26 13:24     ` Ian Jackson
2020-06-29 15:27       ` Tamas K Lengyel
2020-07-01 11:01         ` Ian Jackson [this message]
2020-06-22 18:19 ` [PATCH v3 0/7] Implement support for external IPT monitoring Michał Leszczyński

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=24316.27891.433815.62003@mariner.uk.xensource.com \
    --to=ian.jackson@citrix.com \
    --cc=luwei.kang@intel.com \
    --cc=michal.leszczynski@cert.pl \
    --cc=tamas.k.lengyel@gmail.com \
    --cc=tamas.lengyel@intel.com \
    --cc=wl@xen.org \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Xen-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/xen-devel/0 xen-devel/git/0.git
	git clone --mirror https://lore.kernel.org/xen-devel/1 xen-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 xen-devel xen-devel/ https://lore.kernel.org/xen-devel \
		xen-devel@lists.xenproject.org xen-devel@lists.xen.org
	public-inbox-index xen-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.xenproject.lists.xen-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git