Skip to contentSkip to conversation

openvmtools: new package

PATCH Superseded Karoly Kasza 2 participants 3 messages Apr 4, 2014 – Apr 7, 2014

v1 of 7 Superseded Latest is v7 →

lore
Share & tools
lore

Karoly Kasza

author
#0 lore raw

PATCH 1/1 openvmtools: new package

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

Signed-off-by Karoly Kasza <[email protected]>
---
 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
package/Config.in +1 -0
index e816603..907cb23 100644
@@ -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"
 source "package/owl-linux/Config.in"
 source "package/parted/Config.in"
 source "package/pciutils/Config.in"
package/openvmtools/Config.in new file +92 -0

92 lines not on this page · raw message

package/openvmtools/S98vmtoolsd new file +31 -0
new file mode 100755
index 0000000..4b51e0a
@@ -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"
+	;;
+  restart|reload)
+	;;
+  *)
+	echo "Usage: $0 {start|stop|restart}"
+	exit 1
+esac
+
+exit $?
package/openvmtools/openvmtools-01-has_bsd_printf.patch new file +26 -0
new file mode 100644
index 0000000..a33b67b
@@ -0,0 +1,26 @@
+lib/misc/msgList.c: missing #ifdef
+
+This macro checks for BSD style printf(), which is not present
+when compiling for uClibc. The linked functions are unnecessary in
+this case, and they break compilation.
+
+Signed-off-by: Karoly Kasza <[email protected]>
+
+--- openvmtools-9.4.0-1280544.orig/lib/misc/msgList.c	2013-09-23 17:51:10.000000000 +0200
++++ openvmtools-9.4.0-1280544/lib/misc/msgList.c	2014-04-03 13:42:14.138500061 +0200
+@@ -487,6 +487,7 @@
+    return messages->id;
+ }
+ 
++#ifdef HAS_BSD_PRINTF
+ 
+ /*
+  *----------------------------------------------------------------------
+@@ -566,6 +567,7 @@
+    }
+ }
+ 
++#endif
+ 
+ /*
+  *----------------------------------------------------------------------
package/openvmtools/openvmtools.mk new file +93 -0

93 lines not on this page · raw message

package/openvmtools/shutdown new file +7 -0
new file mode 100755
index 0000000..bca9765
@@ -0,0 +1,7 @@
+#!/bin/sh
+#compatibility script for openvmtools
+if [ "$1" == "-r" ]; then
+/sbin/reboot
+else
+/sbin/poweroff
+fi
Signature · 1 line
1.7.10.4
#1 lore raw

PATCH 1/1 openvmtools: new package

Dear Karoly Kasza,

Thanks a lot for working on this, much appreciated! Here are a few comments below.

Quoted 10 lines · Karoly Kasza

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.

Quoted 2 lines

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.

Quoted 21 lines
Signed-off-by Karoly Kasza <[email protected]>
---
 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
package/Config.in +0 -0
index e816603..907cb23 100644

package/Config.in source "package/on2-8170-modules/Config.in" · line 331

Quoted 4 lines
package/Config.in +1 -0
@@ -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.

Quoted 9 lines
package/Config.in +0 -0
 source "package/owl-linux/Config.in"
 source "package/parted/Config.in"
 source "package/pciutils/Config.in"
package/openvmtools/Config.in new file +0 -0
new file mode 100644
index 0000000..fd9486b

package/openvmtools/Config.in line 6

Quoted 6 lines
package/openvmtools/Config.in new file +6 -0
@@ -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 "#"

package/openvmtools/Config.in line 7

Quoted 1 line
package/openvmtools/Config.in new file +1 -0
+	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.

package/openvmtools/Config.in line 9

Quoted 2 lines
package/openvmtools/Config.in new file +2 -0
+	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.

package/openvmtools/Config.in line 10

Quoted 1 line
package/openvmtools/Config.in new file +1 -0
+	select BR2_PACKAGE_LIBGLIB2 #always needed

No need for "#always needed".

Quoted 6 lines
package/openvmtools/Config.in new file +6 -0
+	help
+	  Open Virtual Machine Tools for VMware guest OS
+
+	  http://open-vm-tools.sourceforge.net/
+
+if BR2_PACKAGE_OPENVMTOOLS

package/openvmtools/Config.in line 26

Quoted 10 lines
package/openvmtools/Config.in new file +10 -0
+
+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.

package/openvmtools/Config.in line 30

Quoted 3 lines
package/openvmtools/Config.in new file +3 -0
+config BR2_PACKAGE_OPENVMTOOLS_PROCPS
+	bool "openvmtools procps support"
+	depends on BR2_PACKAGE_PROCPS

Why a "depends on" here?

package/openvmtools/Config.in line 40

Quoted 10 lines
package/openvmtools/Config.in new file +10 -0
+	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.

Quoted 17 lines
package/openvmtools/Config.in new file +17 -0
+	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

package/openvmtools/Config.in line 67

Quoted 10 lines
package/openvmtools/Config.in new file +10 -0
+	  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.

package/openvmtools/Config.in line 80

Quoted 13 lines
package/openvmtools/Config.in new file +13 -0
+
+#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.

package/openvmtools/Config.in line 81

Quoted 2 lines
package/openvmtools/Config.in new file +2 -0
+
+comment "openvmtools X11 tools are not supported"

Same as above.

Quoted 28 lines
package/openvmtools/Config.in new file +10 -0
+
+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
package/openvmtools/S98vmtoolsd new file +12 -0
new file mode 100755
index 0000000..4b51e0a
@@ -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

package/openvmtools/S98vmtoolsd line 22

Quoted 10 lines
package/openvmtools/S98vmtoolsd new file +10 -0
+		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.

package/openvmtools/openvmtools.mk line 5

Quoted 11 lines
package/openvmtools/openvmtools.mk new file +5 -0
new file mode 100644
index 0000000..1278dae
@@ -0,0 +1,93 @@
+#############################################################
+#
+# openvmtools
+#
+#############################################################

There should be 80 # signs for this header, and an empty line between the header and the first variable.

package/openvmtools/openvmtools.mk line 10

Quoted 5 lines
package/openvmtools/openvmtools.mk new file +5 -0
+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.

package/openvmtools/openvmtools.mk line 11

Quoted 1 line
package/openvmtools/openvmtools.mk new file +1 -0
+OPENVMTOOLS_MAKE_OPT += "CFLAGS+=-Wno-unused-local-typedefs"

This should be part of _CONF_ENV, as follows:

# A comment that explains why this is needed.

OPENVMTOOLS_CONF_ENV = \
	CFLAGS="$(TARGET_CFLAGS) -Wno-unused-local-typedefs"

package/openvmtools/openvmtools.mk line 13

Quoted 2 lines
package/openvmtools/openvmtools.mk new file +2 -0
+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.

package/openvmtools/openvmtools.mk line 15

Quoted 2 lines
package/openvmtools/openvmtools.mk new file +2 -0
+
+#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.

package/openvmtools/openvmtools.mk line 21

Quoted 6 lines
package/openvmtools/openvmtools.mk new file +6 -0
+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.

package/openvmtools/openvmtools.mk line 28

Quoted 7 lines
package/openvmtools/openvmtools.mk new file +7 -0
+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).

Quoted 7 lines
package/openvmtools/openvmtools.mk new file +7 -0
+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)

package/openvmtools/openvmtools.mk line 45

Quoted 10 lines
package/openvmtools/openvmtools.mk new file +10 -0
+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.

package/openvmtools/openvmtools.mk line 58

Quoted 13 lines
package/openvmtools/openvmtools.mk new file +13 -0
+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.

package/openvmtools/openvmtools.mk line 63

Quoted 5 lines
package/openvmtools/openvmtools.mk new file +5 -0
+
+#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

package/openvmtools/openvmtools.mk line 68

Quoted 4 lines
package/openvmtools/openvmtools.mk new file +4 -0
+#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?

package/openvmtools/openvmtools.mk line 73

Quoted 5 lines
package/openvmtools/openvmtools.mk new file +5 -0
+	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.

Quoted 26 lines
package/openvmtools/openvmtools.mk new file +20 -0
+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))
package/openvmtools/shutdown new file +0 -0
new file mode 100755
index 0000000..bca9765

package/openvmtools/shutdown line 7

Quoted 7 lines
package/openvmtools/shutdown new file +7 -0
@@ -0,0 +1,7 @@
+#!/bin/sh
+#compatibility script for openvmtools
+if [ "$1" == "-r" ]; then
+/sbin/reboot
+else
+/sbin/poweroff
+fi

Thanks!

Thomas

Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
author
#2 lore raw

PATCH 1/1 openvmtools: new package

Hello Thomas,

Quoted 16 lines · Thomas Petazzoni

Dear Karoly Kasza,

Thanks a lot for working on this, much appreciated! Here are a few comments below.

Quoted 10 lines · Karoly Kasza

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.

Quoted 10 lines
Quoted 2 lines

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.

Quoted 19 lines
Quoted 5 lines
Signed-off-by Karoly Kasza <[email protected]>
---
 package/Config.in                                  |    1 +
 package/openvmtools/Config.in                      |   92

+++++++++++++++++++

Quoted 3 lines
 package/openvmtools/S98vmtoolsd                    |   31 +++++++
 .../openvmtools-01-has_bsd_printf.patch            |   26 ++++++
 package/openvmtools/openvmtools.mk                 |   93

++++++++++++++++++++

Quoted 5 lines
 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

Quoted 3 lines
 create mode 100644 package/openvmtools/openvmtools.mk
 create mode 100755 package/openvmtools/shutdown
Quoted 13 lines

package/Config.in source "package/on2-8170-modules/Config.in" · line 331

Quoted 9 lines
package/Config.in +1 -0
index e816603..907cb23 100644
@@ -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?

Quoted 20 lines

Review

Quoted 5 lines
package/Config.in +0 -0
 source "package/owl-linux/Config.in"
 source "package/parted/Config.in"
 source "package/pciutils/Config.in"
(quoted diff) +0 -0

b/package/openvmtools/Config.in

package/openvmtools/Config.in line 6

Quoted 11 lines

new file mode 100644
index 0000000..fd9486b

package/openvmtools/Config.in new file +6 -0
@@ -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 "#"

Quoted 6 lines

package/openvmtools/Config.in line 7

Quoted 1 line
package/openvmtools/Config.in new file +1 -0
+     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.

Quoted 34 lines

package/openvmtools/Config.in line 8

Quoted 2 lines
package/openvmtools/Config.in new file +1 -0
+     depends on BR2_LARGEFILE #needed by an include in

lib/include/vmxrpc.h

package/openvmtools/Config.in line 9

Quoted 1 line
package/openvmtools/Config.in new file +1 -0
+     depends on BR2_ENABLE_LOCALE #needed by __locale_t in

lib/misc/codesetOld.c

Ditto.

package/openvmtools/Config.in line 10

Quoted 1 line
package/openvmtools/Config.in new file +1 -0
+     select BR2_PACKAGE_LIBGLIB2 #always needed

No need for "#always needed".

package/openvmtools/Config.in

Quoted 10 lines
package/openvmtools/Config.in new file +10 -0
+     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,

package/openvmtools/Config.in line 22

Quoted 2 lines
package/openvmtools/Config.in new file +2 -0
+#so we warn the user
+comment "openvmtools vmblock-fuse is disabled because these is no

libfuse!"

package/openvmtools/Config.in line 26

Quoted 4 lines
package/openvmtools/Config.in new file +4 -0
+     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:

Quoted 3 lines

Support for vmblock-fuse will be enabled in openvmtools if the libfuse package is selected.

OK

Quoted 24 lines

package/openvmtools/Config.in line 30

Quoted 4 lines
package/openvmtools/Config.in new file +3 -0
+config BR2_PACKAGE_OPENVMTOOLS_PROCPS
+     bool "openvmtools procps support"
+     depends on BR2_PACKAGE_PROCPS

Why a "depends on" here?

package/openvmtools/Config.in

Quoted 9 lines
package/openvmtools/Config.in new file +9 -0
+     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

package/openvmtools/Config.in line 40

Quoted 1 line
package/openvmtools/Config.in new file +1 -0
+     select BR2_PACKAGE_LIBDNET

And a select here?

Also, this is a bit inconsistent with the choice you've made for the libfuse dependency:

Quoted 8 lines

* 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?

Quoted 25 lines
Quoted 11 lines
package/openvmtools/Config.in new file +10 -0
+     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

package/openvmtools/Config.in

Quoted 10 lines
package/openvmtools/Config.in new file +10 -0
+       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.

package/openvmtools/Config.in line 61

Quoted 2 lines
package/openvmtools/Config.in new file +2 -0
+       Only compile this if you are positive that you need it!
+       Also, this is highly untested, so probably you'll need to hack

it.

Quoted 9 lines

package/openvmtools/Config.in line 63

Quoted 2 lines
package/openvmtools/Config.in new file +2 -0
+
+comment "openvmtools older kernel modules needs a Linux kernel to be

built"

package/openvmtools/Config.in line 67

Quoted 3 lines
package/openvmtools/Config.in new file +3 -0
+     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.

Quoted 17 lines
Quoted 4 lines
package/openvmtools/Config.in new file +3 -0
+
+#Does not compile, also needs some hacks in configure...
+#config BR2_PACKAGE_OPENVMTOOLS_ICU

package/openvmtools/Config.in line 80

Quoted 10 lines
package/openvmtools/Config.in new file +10 -0
+#    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.

Quoted 42 lines

package/openvmtools/Config.in line 81

Quoted 3 lines
package/openvmtools/Config.in new file +2 -0
+
+comment "openvmtools X11 tools are not supported"

Same as above.

package/openvmtools/Config.in

Quoted 6 lines
package/openvmtools/Config.in new file +6 -0
+
+endmenu
+
+endif
+
+comment "openvmtools needs a toolchain w/ wchar, threads, RPC,

largefile, locale"

package/openvmtools/Config.in

Quoted 4 lines
package/openvmtools/Config.in new file +4 -0
+     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

Review

Quoted 1 line
(quoted diff) +0 -0

b/package/openvmtools/S98vmtoolsd

Quoted 22 lines

new file mode 100755
index 0000000..4b51e0a

package/openvmtools/S98vmtoolsd new file +17 -0
@@ -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
Quoted 9 lines

package/openvmtools/S98vmtoolsd line 22

Quoted 5 lines
package/openvmtools/S98vmtoolsd new file +5 -0
+     ;;
+  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?

Quoted 4 lines

package/openvmtools/

Quoted 3 lines
package/openvmtools/ renamed from package/openvmtools/openvmtools.mk +0 -0

openvmtools.mk

Quoted 14 lines

package/openvmtools/openvmtools.mk line 5

Quoted 10 lines

new file mode 100644
index 0000000..1278dae

package/openvmtools/openvmtools.mk new file +5 -0
@@ -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.

Quoted 10 lines

package/openvmtools/openvmtools.mk

Quoted 4 lines
package/openvmtools/openvmtools.mk new file +3 -0
+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

package/openvmtools/openvmtools.mk line 10

Quoted 2 lines
package/openvmtools/openvmtools.mk new file +2 -0
+
+OPENVMTOOLS_CONF_OPT += --disable-docs

Not needed, already passed by the autotools-package infrastructure.

True.

Quoted 5 lines

package/openvmtools/openvmtools.mk line 11

Quoted 2 lines
package/openvmtools/openvmtools.mk new file +1 -0
+OPENVMTOOLS_MAKE_OPT += "CFLAGS+=-Wno-unused-local-typedefs"

This should be part of _CONF_ENV, as follows:

OK.

Quoted 5 lines

# A comment that explains why this is needed.

OPENVMTOOLS_CONF_ENV = \
        CFLAGS="$(TARGET_CFLAGS) -Wno-unused-local-typedefs"
Quoted 7 lines

package/openvmtools/openvmtools.mk line 13

Quoted 2 lines
package/openvmtools/openvmtools.mk new file +2 -0
+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.

Quoted 11 lines

package/openvmtools/openvmtools.mk line 15

Quoted 3 lines
package/openvmtools/openvmtools.mk new file +2 -0
+
+#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.

Quoted 9 lines

package/openvmtools/openvmtools.mk

Quoted 7 lines
package/openvmtools/openvmtools.mk new file +6 -0
+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"

Quoted 4 lines

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?

Quoted 10 lines

package/openvmtools/openvmtools.mk

Quoted 8 lines
package/openvmtools/openvmtools.mk new file +7 -0
+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

Quoted 4 lines

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).

Quoted 19 lines
Quoted 7 lines
package/openvmtools/openvmtools.mk new file +7 -0
+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)

package/openvmtools/openvmtools.mk

Quoted 10 lines
package/openvmtools/openvmtools.mk new file +10 -0
+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)

Quoted 5 lines

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.

Quoted 26 lines
Quoted 4 lines
package/openvmtools/openvmtools.mk new file +3 -0
+OPENVMTOOLS_DEPENDENCIES += linux
+OPENVMTOOLS_PRE_CONFIGURE_HOOKS += OPENVMTOOLS_PRE_CONFIGURE_LINUX_FIX
+else

package/openvmtools/openvmtools.mk line 58

Quoted 10 lines
package/openvmtools/openvmtools.mk new file +10 -0
+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.

package/openvmtools/openvmtools.mk line 63

Quoted 5 lines
package/openvmtools/openvmtools.mk new file +5 -0
+
+#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:

Quoted 5 lines
OPENVMTOOLS_CONF_OPT = \
        --without-x \
        --without-gtk2 \
        --without-gtkmm

OK.

Quoted 10 lines

package/openvmtools/openvmtools.mk

Quoted 4 lines
package/openvmtools/openvmtools.mk new file +3 -0
+#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 \

package/openvmtools/openvmtools.mk line 67

Quoted 1 line
package/openvmtools/openvmtools.mk new file +1 -0
+             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).

Quoted 8 lines

package/openvmtools/openvmtools.mk

Quoted 6 lines
package/openvmtools/openvmtools.mk new file +5 -0
+     fi
+endef
+
+define OPENVMTOOLS_PRE_CONFIGURE_DNET_FIX
+     cp -f $(BUILD_DIR)/libdnet-$(LIBDNET_VERSION)/dnet-config

$(TARGET_DIR)/usr/bin

Quoted 6 lines

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?

Quoted 37 lines
Quoted 27 lines
package/openvmtools/openvmtools.mk new file +20 -0
+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))
package/openvmtools/shutdown new file +0 -0
new file mode 100755
index 0000000..bca9765

package/openvmtools/shutdown line 7

Quoted 7 lines
package/openvmtools/shutdown new file +7 -0
@@ -0,0 +1,7 @@
+#!/bin/sh
+#compatibility script for openvmtools
+if [ "$1" == "-r" ]; then
+/sbin/reboot
+else
+/sbin/poweroff
+fi

Thanks!

Quoted 5 lines

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.html>