Re: [PATCH v3 7/7] tools/proctrace: add proctrace tool
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 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.
Re: [PATCH v3 7/7] tools/proctrace: add proctrace tool
On Fri, Jun 26, 2020 at 7:26 AM Ian Jackson 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.
Cheers,
Tamas
Re: [PATCH v3 7/7] tools/proctrace: add proctrace tool
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.
Ian.
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. > > Signed-off-by: Michal Leszczynski > --- > tools/proctrace/COPYING | 339 > tools/proctrace/Makefile| 50 ++ > tools/proctrace/proctrace.c | 158 + > 3 files changed, 547 insertions(+) > create mode 100644 tools/proctrace/COPYING > create mode 100644 tools/proctrace/Makefile > create mode 100644 tools/proctrace/proctrace.c > > diff --git a/tools/proctrace/COPYING b/tools/proctrace/COPYING > new file mode 100644 > index 00..c0a841112c > --- /dev/null > +++ b/tools/proctrace/COPYING > @@ -0,0 +1,339 @@ > + GNU GENERAL PUBLIC LICENSE > +Version 2, June 1991 > + > + Copyright (C) 1989, 1991 Free Software Foundation, Inc. > + 59 Temple Place, Suite 330, Boston, MA 02111-1307 > USA > + Everyone is permitted to copy and distribute verbatim copies > + of this license document, but changing it is not allowed. > + > + Preamble > + > + The licenses for most software are designed to take away your > +freedom to share and change it. By contrast, the GNU General Public > +License is intended to guarantee your freedom to share and change free > +software--to make sure the software is free for all its users. This > +General Public License applies to most of the Free Software > +Foundation's software and to any other program whose authors commit to > +using it. (Some other Free Software Foundation software is covered by > +the GNU Library General Public License instead.) You can apply it to > +your programs, too. > + > + When we speak of free software, we are referring to freedom, not > +price. Our General Public Licenses are designed to make sure that you > +have the freedom to distribute copies of free software (and charge for > +this service if you wish), that you receive source code or can get it > +if you want it, that you can change the software or use pieces of it > +in new free programs; and that you know you can do these things. > + > + To protect your rights, we need to make restrictions that forbid > +anyone to deny you these rights or to ask you to surrender the rights. > +These restrictions translate to certain responsibilities for you if you > +distribute copies of the software, or if you modify it. > + > + For example, if you distribute copies of such a program, whether > +gratis or for a fee, you must give the recipients all the rights that > +you have. You must make sure that they, too, receive or can get the > +source code. And you must show them these terms so they know their > +rights. > + > + We protect your rights with two steps: (1) copyright the software, and > +(2) offer you this license which gives you legal permission to copy, > +distribute and/or modify the software. > + > + Also, for each author's protection and ours, we want to make certain > +that everyone understands that there is no warranty for this free > +software. If the software is modified by someone else and passed on, we > +want its recipients to know that what they have is not the original, so > +that any problems introduced by others will not reflect on the original > +authors' reputations. > + > + Finally, any free program is threatened constantly by software > +patents. We wish to avoid the danger that redistributors of a free > +program will individually obtain patent licenses, in effect making the > +program proprietary. To prevent this, we have made it clear that any > +patent must be licensed for everyone's free use or not licensed at all. > + > + The precise terms and conditions for copying, distribution and > +modification follow. > + > + GNU GENERAL PUBLIC LICENSE > + TERMS AND CONDITIONS FOR COPYING, DISTRIBUTION AND MODIFICATION > + > + 0. This License applies to any program or other work which contains > +a notice placed by the copyright holder saying it may be distributed > +under the terms of this General Public License. The "Program", below, > +refers to any such program or work, and a "work based on the Program" > +means either the Program or any derivative work under copyright law: > +that is to say, a work containing the Program or a portion of it, > +either verbatim or with modifications and/or translated into another > +language. (Hereinafter, translation is included without limitation in > +the term "modification".) Each licensee is addressed as "you". > + > +Activities other than copying, distribution and modification are not > +covered by this License; they are outside its scope. The act of > +running the Program is not restricted, and the output from the Program > +is covered only if its contents constitute a work based on the > +Program (independent of having been made by running the Program). > +Whether that is true
Re: [PATCH v3 7/7] tools/proctrace: add proctrace tool
> +rc = xenforeignmemory_unmap_resource(fmem, fres);
> +
> +if (rc) {
> +fprintf(stderr, "Failed to unmap resource\n");
> +return 1;
> +}
> +
> +rc = xc_vmtrace_pt_disable(xc, domid, vcpu_id);
> +
> +if (rc) {
> +fprintf(stderr, "Failed to call xc_vmtrace_pt_disable\n");
> +return 1;
> +}
Looks like you are missing an xc_interface_close here.
> +
> +return 0;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> --
> 2.20.1
>
>
[PATCH v3 7/7] tools/proctrace: add proctrace tool
Add an demonstration tool that uses xc_vmtrace_* calls in order to manage external IPT monitoring for DomU. Signed-off-by: Michal Leszczynski --- tools/proctrace/COPYING | 339 tools/proctrace/Makefile| 50 ++ tools/proctrace/proctrace.c | 158 + 3 files changed, 547 insertions(+) create mode 100644 tools/proctrace/COPYING create mode 100644 tools/proctrace/Makefile create mode 100644 tools/proctrace/proctrace.c diff --git a/tools/proctrace/COPYING b/tools/proctrace/COPYING new file mode 100644 index 00..c0a841112c --- /dev/null +++ b/tools/proctrace/COPYING @@ -0,0 +1,339 @@ + GNU GENERAL PUBLIC LICENSE + Version 2, June 1991 + + Copyright (C) 1989, 1991 Free Software Foundation, Inc. + 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + Everyone is permitted to copy and distribute verbatim copies + of this license document, but changing it is not allowed. + + Preamble + + The licenses for most software are designed to take away your +freedom to share and change it. By contrast, the GNU General Public +License is intended to guarantee your freedom to share and change free +software--to make sure the software is free for all its users. This +General Public License applies to most of the Free Software +Foundation's software and to any other program whose authors commit to +using it. (Some other Free Software Foundation software is covered by +the GNU Library General Public License instead.) You can apply it to +your programs, too. + + When we speak of free software, we are referring to freedom, not +price. Our General Public Licenses are designed to make sure that you +have the freedom to distribute copies of free software (and charge for +this service if you wish), that you receive source code or can get it +if you want it, that you can change the software or use pieces of it +in new free programs; and that you know you can do these things. + + To protect your rights, we need to make restrictions that forbid +anyone to deny you these rights or to ask you to surrender the rights. +These restrictions translate to certain responsibilities for you if you +distribute copies of the software, or if you modify it. + + For example, if you distribute copies of such a program, whether +gratis or for a fee, you must give the recipients all the rights that +you have. You must make sure that they, too, receive or can get the +source code. And you must show them these terms so they know their +rights. + + We protect your rights with two steps: (1) copyright the software, and +(2) offer you this license which gives you legal permission to copy, +distribute and/or modify the software. + + Also, for each author's protection and ours, we want to make certain +that everyone understands that there is no warranty for this free +software. If the software is modified by someone else and passed on, we +want its recipients to know that what they have is not the original, so +that any problems introduced by others will not reflect on the original +authors' reputations. + + Finally, any free program is threatened constantly by software +patents. We wish to avoid the danger that redistributors of a free +program will individually obtain patent licenses, in effect making the +program proprietary. To prevent this, we have made it clear that any +patent must be licensed for everyone's free use or not licensed at all. + + The precise terms and conditions for copying, distribution and +modification follow. + + GNU GENERAL PUBLIC LICENSE + TERMS AND CONDITIONS FOR COPYING, DISTRIBUTION AND MODIFICATION + + 0. This License applies to any program or other work which contains +a notice placed by the copyright holder saying it may be distributed +under the terms of this General Public License. The "Program", below, +refers to any such program or work, and a "work based on the Program" +means either the Program or any derivative work under copyright law: +that is to say, a work containing the Program or a portion of it, +either verbatim or with modifications and/or translated into another +language. (Hereinafter, translation is included without limitation in +the term "modification".) Each licensee is addressed as "you". + +Activities other than copying, distribution and modification are not +covered by this License; they are outside its scope. The act of +running the Program is not restricted, and the output from the Program +is covered only if its contents constitute a work based on the +Program (independent of having been made by running the Program). +Whether that is true depends on what the Program does. + + 1. You may copy and distribute verbatim copies of the Program's +source code as you receive it, in any medium, provided that you +conspicuously and appropriately publish on each copy an appropriate +copyright notice and
