Seems like patch 1 and 2 can/should be combined...
On 05/18/2012 12:38 PM, Sean Bruno wrote:
> ---
> .gitignore | 10 +-
> INSTALL | 365
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> configure.ac | 9 +-
> src/Makefile.am | 7 ++
> src/drv_fbsd.c | 71 +----------
> 5 files changed, 392 insertions(+), 70 deletions(-)
> create mode 100644 INSTALL
>
> diff --git a/.gitignore b/.gitignore
> index 91b3a7c..d46b950 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -8,16 +8,16 @@
> *.pdf
> *.spec
> *~
> -/.git-module-status
> .deps/
> .libs/
> -/build-aux
> +/.git-module-status
> /ChangeLog
> -Makefile
> -Makefile.in
> /TAGS
> /aclocal.m4
> /autom4te.cache/
> +/build-aux
> +Makefile
> +Makefile.in
> build/
> config.guess
> config.h
Any specific reason for the churn in this file? I don't see any useful
change, just movement. You should try to keep unrelated stuff out of
your patches, to avoid confusion and lower the possibility of merge
conflicts.
> @@ -43,3 +43,5 @@ stamp-h1
> tests/test-debian
> tests/test-redhat
> ylwrap
> +/GNUmakefile
> +/maint.mk
> diff --git a/INSTALL b/INSTALL
> new file mode 100644
> index 0000000..7d1c323
> --- /dev/null
> +++ b/INSTALL
> @@ -0,0 +1,365 @@
> +Installation Instructions
> +*************************
[...]
I'm not sure why your tree didn't already have an INSTALL file - it's
there in git master. Even if it hadn't been, a patch to add support for
a new platform wouldn't be the proper place to add it.
> diff --git a/configure.ac b/configure.ac
> index c1060f3..2607a96 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -80,6 +80,9 @@ fi
> if test "x$with_driver" = "xcheck" && test -f /etc/suse-release ; then
> with_driver=suse
> fi
> +if test "x$with_driver" = "xcheck" && test -f /usr/include/sys/param.h ; then
> + with_driver=fbsd
> +fi
You really need a test that is less generic. Is there some file in /etc
(or somewhere else) that is on every freebsd installation, and never on
anything else?
> if test "x$with_driver" = "xcheck" ; then
> AC_MSG_ERROR([Cannot detect network driver, use --with-driver=NAME to
> select implementation])
> fi
> @@ -91,6 +94,7 @@ AM_CONDITIONAL([NETCF_DRIVER_REDHAT], test "x$with_driver"
> = "xredhat")
> AM_CONDITIONAL([NETCF_DRIVER_DEBIAN], test "x$with_driver" = "xdebian")
> AM_CONDITIONAL([NETCF_DRIVER_SUSE], test "x$with_driver" = "xsuse")
> AM_CONDITIONAL([NETCF_DRIVER_MSWINDOWS], test "x$with_driver" = "xmswindows")
> +AM_CONDITIONAL([NETCF_DRIVER_FBSD], test "x$with_driver" = "xfbsd")
>
> if test "x$with_driver" = "xredhat"; then
> AC_DEFINE_UNQUOTED([NETCF_TRANSACTION],
> @@ -103,7 +107,10 @@ AM_CONDITIONAL([NETCF_INIT_SCRIPT_RED_HAT],
> if test "x$with_driver" != "xmswindows"
> then
> PKG_CHECK_MODULES([LIBAUGEAS], [augeas >= 0.5.0])
> - PKG_CHECK_MODULES([LIBNL], [libnl-1])
> + if test "x$with_driver" != "xfbsd"
> + then
> + PKG_CHECK_MODULES([LIBNL], [libnl-1])
> + fi
Does FreeBSD have a package for libnl version 3? If so, I would
recommend having it use that version from the beginning (libnl3 support
was just recently added to both netcf and libvirt).
> fi
>
> NETCF_LIBDEPS=$(echo $LIBAUGEAS_LIBS $LIBEXSLT_LIBS $LIBXSLT_LIBS
> $LIBXML_LIBS $LIBNL_LIBS)
> diff --git a/src/Makefile.am b/src/Makefile.am
> index b5c732a..484ba97 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -16,6 +16,7 @@ noinst_PROGRAMS = ncftransform
> endif
>
> DRIVER_SOURCES_COMMON = dutil.h dutil.c
> +DRIVER_SOURCES_FBSD = dutil_fbsd.h dutil_fbsd.c drv_fbsd.c
> DRIVER_SOURCES_LINUX = dutil_linux.h dutil_linux.c
> DRIVER_SOURCES_MSWINDOWS = dutil_mswindows.h dutil_mswindows.c
> drv_mswindows.c
> DRIVER_SOURCES_REDHAT = drv_redhat.c
> @@ -26,6 +27,7 @@ EXTRA_DIST = netcf_public.syms \
> netcf_private.syms \
> netcf-transaction.init.sh \
> $(DRIVER_SOURCES_COMMON) \
> + $(DRIVER_SOURCES_FBSD) \
> $(DRIVER_SOURCES_LINUX) \
> $(DRIVER_SOURCES_MSWINDOWS) \
> $(DRIVER_SOURCES_REDHAT) \
> @@ -55,6 +57,11 @@ DRIVER_SOURCES = \
> $(DRIVER_SOURCES_COMMON) \
> $(DRIVER_SOURCES_MSWINDOWS)
> endif
> +if NETCF_DRIVER_FBSD
> +DRIVER_SOURCES = \
> + $(DRIVER_SOURCES_COMMON) \
> + $(DRIVER_SOURCES_FBSD)
> +endif
>
> BUILT_SOURCES = datadir.h netcf.syms
>
> diff --git a/src/drv_fbsd.c b/src/drv_fbsd.c
> index 64b0e98..7fd3025 100644
> --- a/src/drv_fbsd.c
> +++ b/src/drv_fbsd.c
> @@ -22,17 +22,16 @@ int drv_init(struct netcf *ncf) {
>
>
> void drv_close(struct netcf *ncf) {
> - if (ncf == NULL || ncf->driver == NULL)
> - return;
> - FREE(ncf->driver);
> -}
>
> + return;
>
> -void drv_entry(struct netcf *ncf ATTRIBUTE_UNUSED) {
All of these functions seem to have been created with one kind of stub
in PATCH 1/5, then modified to a different kind of stub in 2/5. I would
rather they be added with proper content right away; or, if they're
going to be temporarily stubbed out, do it once, and don't change it
until you put proper code into it. Of course in this case you are adding
a completely new platform, so I think it's okay if the first couple
patches don't successfully build on FreeBSD (since that's not a
regression :-)) as long as the patches don't break existing platforms.
> }
>
> -static PIP_ADAPTER_ADDRESSES build_adapter_table(struct netcf *ncf) {
> - return NULL;
> +
> +void build_adapter_table(struct netcf *ncf) {
> +
> + return;
> +
> }
>
> static int list_interface_ids(struct netcf *ncf,
> @@ -55,78 +54,40 @@ int drv_num_of_interfaces(struct netcf *ncf, unsigned int
> flags) {
>
> struct netcf_if *drv_lookup_by_name(struct netcf *ncf, const char *name) {
> ERR_THROW(1 == 1, ncf, EOTHER, "not implemented on this platform");
> -
> - return nif;
> }
>
> const char *drv_mac_string(struct netcf_if *nif) {
> - struct netcf *ncf = nif->ncf;
> ERR_THROW(1 == 1, ncf, EOTHER, "not implemented on this platform");
> - error:
> - free(adapter);
> - free(buf);
> - return nif->mac;
I'm pretty sure this wouldn't build anyway - any function that uses an
ERR_* macro must have an error: label.
_______________________________________________
netcf-devel mailing list
netcf-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/netcf-devel
