[Buildroot] [PATCH 1/1] openvmtools: new package

Mon Apr 7 09:25:04 UTC 2014

Hello Thomas,

On Sun, Apr 6, 2014 at 3:23 PM, Thomas Petazzoni <
thomas.petazzoni at free-electrons.com> wrote:

> Dear Karoly Kasza,
>
> Thanks a lot for working on this, much appreciated! Here are a few
> comments below.
>
> On Fri,  4 Apr 2014 10:12:13 +0200, Karoly Kasza wrote:
> > Hi,
> >
> > this patch adds the latest stable open-vm-tools to buildroot.
> > (http://open-vm-tools.sourceforge.net/)
> > It should be compatible with all 3 libc implementations.
> > Also Contains a small patch for the source, a SysV init script and
> > a small shutdown script to make the VM power options usable from
> > the host OS.
> > Addresses Bugzilla bug 3991.
> >
> > BR
> > Karoly
>
> This part of your e-mail would become part of the git commit log
> forever, so it shouldn't contain "personal" informations. If you want
> to include personal informations, they should either be in an
> introduction e-mail separate from the patch itself (see "git send-email
> --cover"), or below the "---" sign after your Signed-off-by line.
>
>
OK, I'm new to git.

> >
> > Signed-off-by: Karoly Kasza <kaszak at gmail.com>
> > ---
> >  package/Config.in                                  |    1 +
> >  package/openvmtools/Config.in                      |   92
> +++++++++++++++++++
> >  package/openvmtools/S98vmtoolsd                    |   31 +++++++
> >  .../openvmtools-01-has_bsd_printf.patch            |   26 ++++++
> >  package/openvmtools/openvmtools.mk                 |   93
> ++++++++++++++++++++
> >  package/openvmtools/shutdown                       |    7 ++
> >  6 files changed, 250 insertions(+)
> >  create mode 100644 package/openvmtools/Config.in
> >  create mode 100755 package/openvmtools/S98vmtoolsd
> >  create mode 100644
> package/openvmtools/openvmtools-01-has_bsd_printf.patch
> >  create mode 100644 package/openvmtools/openvmtools.mk
> >  create mode 100755 package/openvmtools/shutdown
> >
> > diff --git a/package/Config.in b/package/Config.in
> > index e816603..907cb23 100644
> > --- a/package/Config.in
> > +++ b/package/Config.in
> > @@ -328,6 +328,7 @@ source "package/on2-8170-modules/Config.in"
> >  source "package/open2300/Config.in"
> >  source "package/openocd/Config.in"
> >  source "package/openpowerlink/Config.in"
> > +source "package/openvmtools/Config.in"
>
> Is openvmtools really related to Hardware handling? It should probably
> be under "System tools" or something like that.
>

This is a decision to make. I put it there, because it is basically a set
of virtual hardware handling tools.
If you like it better at System tools, I'll put it there, no problem.
Should I replace it?

>
> >  source "package/owl-linux/Config.in"
> >  source "package/parted/Config.in"
> >  source "package/pciutils/Config.in"
> > diff --git a/package/openvmtools/Config.in
> b/package/openvmtools/Config.in
> > new file mode 100644
> > index 0000000..fd9486b
> > --- /dev/null
> > +++ b/package/openvmtools/Config.in
> > @@ -0,0 +1,92 @@
> > +config BR2_PACKAGE_OPENVMTOOLS
> > +     bool "openvmtools"
> > +     depends on BR2_i386 || BR2_x86_64
> > +     depends on BR2_USE_MMU #libglib2
> > +     depends on BR2_USE_WCHAR #libglib2
> > +     depends on BR2_TOOLCHAIN_HAS_THREADS #libglib2
>
> One space after "#"
>
> > +     depends on BR2_TOOLCHAIN_HAS_NATIVE_RPC #needed by an include in
> lib/include/vmxrpc.h
>
> For some reason, we generally don't document why these
> toolchain dependencies are needed.
>

I think it would be a good idea for later reference. I can of course remove
them.

>
> > +     depends on BR2_LARGEFILE #needed by an include in
> lib/include/vmxrpc.h
> > +     depends on BR2_ENABLE_LOCALE #needed by __locale_t in
> lib/misc/codesetOld.c
>
> Ditto.
>
> > +     select BR2_PACKAGE_LIBGLIB2 #always needed
>
> No need for "#always needed".
>
> > +     help
> > +       Open Virtual Machine Tools for VMware guest OS
> > +
> > +       http://open-vm-tools.sourceforge.net/
> > +
> > +if BR2_PACKAGE_OPENVMTOOLS
> > +
> > +menu "openvmtools options"
> > +
> > +#This can't be disabled by configure, will compile itself if libfuse is
> found,
> > +#so we warn the user
> > +comment "openvmtools vmblock-fuse is disabled because these is no
> libfuse!"
> > +     depends on !BR2_PACKAGE_LIBFUSE
> > +
> > +comment "openvmtools vmblock-fuse is enabled because libfuse is found!"
> > +     depends on BR2_PACKAGE_LIBFUSE
>
> Such comments are not necessary. You may want to include some
> explanation in the help text of the main option, such as:
>
>         Support for vmblock-fuse will be enabled in openvmtools if the
>         libfuse package is selected.
>

OK

>
> > +config BR2_PACKAGE_OPENVMTOOLS_PROCPS
> > +     bool "openvmtools procps support"
> > +     depends on BR2_PACKAGE_PROCPS
>
> Why a "depends on" here?
>
> > +     help
> > +       Enable support for procps / meminfo
> > +
> > +comment "openvmtools procps support needs procps"
> > +     depends on !BR2_PACKAGE_PROCPS
> > +
> > +config BR2_PACKAGE_OPENVMTOOLS_DNET
> > +     bool "openvmtools dnet support"
> > +     depends on BR2_INET_IPV6 #needed by
> services/plugins/guestInfo/getlib/guestInfoPosix.c
> > +     select BR2_PACKAGE_LIBDNET
>
> And a select here?
>
> Also, this is a bit inconsistent with the choice you've made for the
> libfuse dependency:
>
>  * Either you add explicit sub-options under the openvmtools options,
>    for the various optional features, and you make them "select" the
>    appropriate dependencies.
>
>  * Or you make these optional features automatically enabled when the
>    necessary set of dependencies are available, as you did for
>    vmblock-fuse above.
>

I tried to use these as stated in Buildroot manual Section 7.2.2 "Choosing
depends on or select"
Select for libraries - like libdnet,
and depends on for packages the user should be aware of. - Procps is a
package that is only enabled
if you select "Show packages that are also provided by busybox".
I can of course make both selected, but is it a good idea?

>
> > +     help
> > +       Enable support for libdnet / nicinfo
> > +
> > +comment "openvmtools dnet support needs a toolchain w/ IPv6"
> > +     depends on !BR2_INET_IPV6
> > +
> > +config BR2_PACKAGE_OPENVMTOOLS_PAM
> > +     bool "openvmtools PAM support"
> > +     select BR2_PACKAGE_LINUX_PAM
> > +     help
> > +       Support for PAM in openvmtools
> > +
> > +config BR2_PACKAGE_OPENVMTOOLS_MODULES
> > +     bool "openvmtools older kernel modules - SEE HELP!"
> > +     depends on BR2_LINUX_KERNEL
> > +     help
> > +       Compile older Linux kernel modules for openvmtools
> > +       This is absolutely NOT NEEDED for recent kernels, as they already
> > +       include open-vm-tools modules.
> > +       Better to choose this in a current Linux kernel's config, not
> here.
> > +       Only compile this if you are positive that you need it!
> > +       Also, this is highly untested, so probably you'll need to hack
> it.
> > +
> > +comment "openvmtools older kernel modules needs a Linux kernel to be
> built"
> > +     depends on !BR2_LINUX_KERNEL
> > +
> > +comment "openvmtools ICU locales are not supported"
>
> Don't put comment for things that are not supported.
>

OK, it was for later reference.

>
> > +
> > +#Does not compile, also needs some hacks in configure...
> > +#config BR2_PACKAGE_OPENVMTOOLS_ICU
> > +#    bool "openvmtools icu locales suport"
> > +#    depends on BR2_INSTALL_LIBSTDCPP #needed by icu
> > +#    select BR2_PACKAGE_ICU
> > +#    help
> > +#      Use icu for openvmtools locales
> > +#      I would only recommend to use this only if you already has icu,
> > +#      as it is large and requires a C++ compiler
> > +
> > +#comment "openvmtools icu locales support needs a toolchain w/ C++"
> > +#    depends on !BR2_INSTALL_LIBSTDCPP
>
> Don't keep commented code.
>

It looked like a good idea if anybody else wants to add the support in the
future.

>
> > +
> > +comment "openvmtools X11 tools are not supported"
>
> Same as above.
>
> > +
> > +endmenu
> > +
> > +endif
> > +
> > +comment "openvmtools needs a toolchain w/ wchar, threads, RPC,
> largefile, locale"
> > +     depends on BR2_i386 || BR2_x86_64
> > +     depends on BR2_USE_MMU
> > +     depends on !BR2_USE_WCHAR ||!BR2_TOOLCHAIN_HAS_THREADS || \
> > +     !BR2_TOOLCHAIN_HAS_NATIVE_RPC || !BR2_LARGEFILE ||
> !BR2_ENABLE_LOCALE
> > diff --git a/package/openvmtools/S98vmtoolsd
> b/package/openvmtools/S98vmtoolsd
> > new file mode 100755
> > index 0000000..4b51e0a
> > --- /dev/null
> > +++ b/package/openvmtools/S98vmtoolsd
> > @@ -0,0 +1,31 @@
> > +#!/bin/sh
> > +#
> > +# Start vmtoolsd
> > +#
> > +
> > +PID=/var/run/vmtoolsd.pid
> > +
> > +case "$1" in
> > +  start)
> > +     echo -n "Starting vmtoolsd: "
> > +     /usr/bin/vmtoolsd -b $PID
> > +     if [ $? != 0 ]; then
> > +             echo "FAILED"
> > +             exit 1
> > +     else
> > +             echo "OK"
> > +     fi
> > +     ;;
> > +  stop)
> > +     echo -n "Stopping vmtoolsd: "
> > +     kill `cat $PID`
> > +     echo "OK"
>
> I believe it would be better to use start-stop-daemon here. Have a look
> at package/connman/S45connman for an example.
>

I know start-stop-daemon, but I don't really see its advantage over this
simple script.
Stopping only occurs when shutting down the VM, and the daemon creates its
PID when starting in bg mode.
I can rewrite it, but is it needed?

>
>
> > diff --git a/package/openvmtools/openvmtools.mk b/package/openvmtools/
> openvmtools.mk
> > new file mode 100644
> > index 0000000..1278dae
> > --- /dev/null
> > +++ b/package/openvmtools/openvmtools.mk
> > @@ -0,0 +1,93 @@
> > +#############################################################
> > +#
> > +# openvmtools
> > +#
> > +#############################################################
>
> There should be 80 # signs for this header, and an empty line between
> the header and the first variable.
>

I copied it from some other package, may be it was outdated.
I'll correct this.

>
> > +OPENVMTOOLS_VERSION = 9.4.0-1280544
> > +OPENVMTOOLS_SOURCE = open-vm-tools-$(OPENVMTOOLS_VERSION).tar.gz
> > +OPENVMTOOLS_SITE =
> http://downloads.sourceforge.net/project/open-vm-tools/open-vm-tools/stable-9.4.x
> > +
> > +OPENVMTOOLS_CONF_OPT += --disable-docs
>
> Not needed, already passed by the autotools-package infrastructure.
>

True.

>
> > +OPENVMTOOLS_MAKE_OPT += "CFLAGS+=-Wno-unused-local-typedefs"
>
> This should be part of _CONF_ENV, as follows:
>

OK.

>
> # A comment that explains why this is needed.
> OPENVMTOOLS_CONF_ENV = \
>         CFLAGS="$(TARGET_CFLAGS) -Wno-unused-local-typedefs"
>
> > +OPENVMTOOLS_DEPENDENCIES += libglib2
> > +OPENVMTOOLS_POST_INSTALL_TARGET_HOOKS +=
> OPENVMTOOLS_POST_INSTALL_TARGET_THINGIES
>
> The registration of a hook should be close (usually right after) the
> hook definition.
>

I'll replace it.

>
> > +
> > +#If libfuse is found, vmblock-fuse is always compiled, we just set a
> sequence
>
> "set a sequence" is a bit confusing, and is not part of the typical
> Buildroot terminology. Instead this could be something like:
>
> # When libfuse is available, openvmtools can build vmblock-fuse, so
> # make sure that libfuse gets built first.
>

OK.

>
> > +ifeq ($(BR2_PACKAGE_LIBFUSE),y)
> > +OPENVMTOOLS_DEPENDENCIES += libfuse
> > +endif
> > +
> > +ifeq ($(BR2_PACKAGE_OPENVMTOOLS_PROCPS),y)
> > +OPENVMTOOLS_CONF_OPT += --with-procps LDFLAGS="-L$(TARGET_DIR)/lib
> -L$(TARGET_DIR)/usr/lib"
>
> No: you should instead change the procps package (as part of a separate
> patch) to make it install its libraries to the staging directory. Then
> you won't need those special LDFLAGS.
>

I didn't want to patch anybody else's package just to make mine more
comfortable.
I think the special LDFLAGS exists for a reason, and this can be.
If you want me to do it, I can create a patch for procps.
I'd stay with LDFLAGS, or should I create a procps patch?

>
> > +OPENVMTOOLS_DEPENDENCIES += procps
> > +else
> > +OPENVMTOOLS_CONF_OPT += --without-procps
> > +endif
> > +
> > +ifeq ($(BR2_PACKAGE_OPENVMTOOLS_DNET),y)
> > +OPENVMTOOLS_CONF_OPT += --with-dnet
> CUSTOM_DNET_CPPFLAGS=-I$(TARGET_DIR)/usr/include
>
> libdnet headers are installed to the staging directory, so there should
> be no need for these CUSTOM_DNET_CPPFLAGS, and if they are really
> needed, they should point to $(STAGING_DIR).
>

They are needed, for some reason configure in open-vm-tools won't find
libdnet any other way. I'll try it with $(STAGING_DIR).

> > +OPENVMTOOLS_DEPENDENCIES += libdnet
> > +OPENVMTOOLS_PRE_CONFIGURE_HOOKS += OPENVMTOOLS_PRE_CONFIGURE_DNET_FIX
> > +else
> > +OPENVMTOOLS_CONF_OPT += --without-dnet
> > +endif
> > +
> > +ifeq ($(BR2_PACKAGE_OPENVMTOOLS_PAM),y)
> > +OPENVMTOOLS_CONF_OPT += --with-pam
> > +OPENVMTOOLS_DEPENDENCIES += linux-pam
> > +else
> > +OPENVMTOOLS_CONF_OPT += --without-pam
> > +endif
> > +
> > +#For older kernels only!
> > +#See http://sourceforge.net/p/open-vm-tools/mailman/message/30113760/for some clue
> > +ifeq ($(BR2_PACKAGE_OPENVMTOOLS_MODULES),y)
> > +OPENVMTOOLS_CONF_OPT += --with-kernel-modules
> --with-linuxdir=$(BUILD_DIR)/linux-$(LINUX_HEADERS_VERSION)
>
> Use $(LINUX_DIR) instead of
> $(BUILD_DIR)/linux-$(LINUX_HEADERS_VERSION). Especially since
> $(LINUX_HEADERS_VERSION) is the version of the kernel headers, not the
> version of the kernel being built.
>

OK.

>
> > +OPENVMTOOLS_DEPENDENCIES += linux
> > +OPENVMTOOLS_PRE_CONFIGURE_HOOKS += OPENVMTOOLS_PRE_CONFIGURE_LINUX_FIX
> > +else
> > +OPENVMTOOLS_CONF_OPT += --without-kernel-modules
> > +endif
> > +
> > +#Does not configure or compile, needs hacks, also not really needed...
> > +#ifeq ($(BR2_PACKAGE_OPENVMTOOLS_ICU),y)
> > +#OPENVMTOOLS_CONF_OPT += --with-icu
> > +#OPENVMTOOLS_DEPENDENCIES = icu
> > +#else
> > +OPENVMTOOLS_CONF_OPT += --without-icu
> > +#endif
>
> Don't keep commented code.

> > +
> > +#No X11 suport (yet)
> > +OPENVMTOOLS_CONF_OPT += --without-x
> > +OPENVMTOOLS_CONF_OPT += --without-gtk2
> > +OPENVMTOOLS_CONF_OPT += --without-gtkmm
>
> Add these at the top of the file, like:
>
> OPENVMTOOLS_CONF_OPT = \
>         --without-x \
>         --without-gtk2 \
>         --without-gtkmm
>

OK.

>
> > +#create a "build" directory symlink if building kernel modules
> > +define OPENVMTOOLS_PRE_CONFIGURE_LINUX_FIX
> > +     if [ ! -e $(BUILD_DIR)/linux-$(LINUX_HEADERS_VERSION)/build ];
> then \
> > +             ln -s . $(BUILD_DIR)/linux-$(LINUX_HEADERS_VERSION)/build;
> \
>
> Huh, why?
>

Configure didn't find it other whence. I'll check it with $(LINUX_DIR).

>
> > +     fi
> > +endef
> > +
> > +define OPENVMTOOLS_PRE_CONFIGURE_DNET_FIX
> > +     cp -f $(BUILD_DIR)/libdnet-$(LIBDNET_VERSION)/dnet-config
> $(TARGET_DIR)/usr/bin
>
> Arrggh, no! It is absolutely horrible for package A to install files
> from package B into the target directory. In addition to this,
> dnet-config is already installed into $(STAGING_DIR)/usr/bin/.
> Normally, the openvmtools configure script should take an environment
> variable that allows to set the location of the dnet-config script.
>

OK, I don't know if such environment variable exists. I'll look for it.
What should I do if no such variable exist? Patch libdnet or this package?

>
> > +endef
> > +
> > +define OPENVMTOOLS_POST_INSTALL_TARGET_THINGIES
> > +#needed by lib/system/systemLinux.c (or will cry in /var/log/messages)
> > +     if [ ! -e $(TARGET_DIR)/etc/lfs-release ]; then \
> > +             ln -s os-release $(TARGET_DIR)/etc/lfs-release; \
> > +     fi
> > +#for Guest OS restart/shutdown
> > +     if [ ! -e $(TARGET_DIR)/sbin/shutdown ]; then \
> > +             $(INSTALL) -D -m 755 package/openvmtools/shutdown \
> > +                     $(TARGET_DIR)/sbin/shutdown; \
> > +     fi
> > +endef
> > +
> > +define OPENVMTOOLS_INSTALL_INIT_SYSV
> > +     $(INSTALL) -D -m 755 package/openvmtools/S98vmtoolsd \
> > +             $(TARGET_DIR)/etc/init.d/S98vmtoolsd
> > +endef
> > +
> > +$(eval $(autotools-package))
> > diff --git a/package/openvmtools/shutdown b/package/openvmtools/shutdown
> > new file mode 100755
> > index 0000000..bca9765
> > --- /dev/null
> > +++ b/package/openvmtools/shutdown
> > @@ -0,0 +1,7 @@
> > +#!/bin/sh
> > +#compatibility script for openvmtools
> > +if [ "$1" == "-r" ]; then
> > +/sbin/reboot
> > +else
> > +/sbin/poweroff
> > +fi
>
> Thanks!
>
> Thomas
> --
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com

Allow me a few comments:
- buildroot is not an easy system to work with, and the _expected_
structure of *.mk and config.in files are very undocumented.
For example the $(LINUX_DIR) macro isn't mentioned in the user manual. This
makes it hard to develop onto and most of your remarks are resulting from
this.
- I don't know why you don't like comments in source code, as they make it
more easy to modify for outsiders. That's why I used them - for the future
reference. Also, documenting for the dependencies should be a good idea. It
is really the purpose of the # tag.

Please answer my few in-line questions then I'll send a v2 patch in a few
days!

Kind regards,
Karoly
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20140407/6c25b0be/attachment-0002.html>