[netcf-devel] [PATCH 2/2] support systemd-based netcf-transaction

Laine Stump laine at laine.org
Wed Jan 22 10:33:49 UTC 2014


On 01/21/2014 09:14 PM, Eric Blake wrote:
> On 01/21/2014 05:55 AM, Laine Stump wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1044681
>>
>> The netcf-transaction script is used in two ways:
>>
>> 1) It is called by libnetcf.so to begin, commit, and manually rollback
>> a set of network interface config changes.
>>
>> 2) It is called as part of normal system startup to automatically
>> rollback any config changes that are part of a netcf transaction that
>> hasn't yet been committed. (in other words, if you reboot after
>> calling ncf_change_begin() but before calling ncf_change_commit(), all
>> the changes you made subsequent to ncf_change_begin() will be erased).
>>
>> NB: the location of the netcf-transaction script, which src/drv_*.c
>> find via the NETCF_TRANSACTION #define is currently hardcoded to look
>> in /usr/libexec. Ideally, this should use $libexecdir, but that
>> resolves to "${exec_prefix}/libexec" at configure time. To get the
>> desired results, we need to update gnulib and use gnulib's configmake
>> package, this way we'll end up with a NETCF_TRANSACTION that will be
>> set at the time make is run rather than configure. This is left as a
>> separate patch though, so that the initial support for systemd can be
>> backported to older releases of netcf (a gnulib update shouldn't be
>> backported to the maintenance branch of an already-cut release).
> And I'm working on that followup :)
>
>
>> +    *)
>> +       AC_MSG_ERROR([Unknown system initscript flavour $with_sysinit])
> Should I even care about US vs. UK spelling?


I'm pretty sure I used U.S. spelling in the comments. This is one of the
cases where I cut-pasted and didn't notice the u.


>
>> +    ;;
>> +esac
>> +if test "x$with_sysinit" = "xcheck" ; then
>> +  with_sysinit=none
>> +fi
>> +
>> +AM_CONDITIONAL([NETCF_USE_INITSCRIPTS], test "$with_sysinit" = "initscripts")
>> +AM_CONDITIONAL([NETCF_USE_SYSTEMD], test "$with_sysinit" = "systemd")
> Libvirt allowed initscripts+systemd, for installing both methods
> simultaneously; but I'm okay with not adding that complexity here in netcf.

Does it? Or are you thinking of "systemd+redhat"? My assumption was that
doing that would build just the systemd files, but dtrt for Red
Hat/Fedora/CentOS. I don't really see the utility of building/installing
both initscripts and systemd files. (I guess part of the problem is that
libvirt names it "redhat" instead of "initscripts", thus clouding the
issue of exactly what they're talking about.)


>
>> +AC_MSG_RESULT($with_sysinit)
>> +if test "$with_sysinit" != "none" && test "$with_driver" != "redhat"; then
>> +  AC_MSG_ERROR([netcf does not have support for $with_sysinit combined with the $with_driver driver])
>> +fi
> Maybe a bit over-restrictive, but it will force other distros to send
> patches to get their flavor supported :)


And it will prevent the "download source, browse config stuff for 5
minutes, then set some options and build" people from believing that
they've successfully made a build with support for netcf-transaction on
their distro, when really configure is just ignoring that part of the
options (or possibly they're installing some Red Hat/Fedora specific
files which will never be used on their Ubuntu/Suse system)


>> +++ b/src/Makefile.am
>> @@ -16,7 +16,9 @@ include_HEADERS = netcf.h
>>  lib_LTLIBRARIES = libnetcf.la
>>  
>>  bin_PROGRAMS = ncftool
>> -
>> +if NETCF_DRIVER_REDHAT
>> +libexec_SCRIPTS = netcf-transaction.sh
>> +endif
> Rather, shouldn't this libexec_SCRIPTS be dependent on
> NETCF_USE_INITSCRIPTS || NETCF_USE_SYSTEMD?  

Yes, you're correct, and I'm fixing that before I push.

> Just because
> NETCF_DRIVER_REDHAT happens to be the only case where we need the helper
> script, it seems like it is less fragile to make the installation of the
> helper script depend on the init script mode chosen, rather than the
> driver interaction that happens to use init scripts.  But again, it
> matches your configure.ac, and if anyone else cares, they can fix this
> at the same time as changing configure.ac to allow their distro.
>
>
>> +netcf-transaction.init: netcf-transaction.init.in \
>> +                        $(top_builddir)/config.status
>> +	$(AM_V_GEN)sed \
>> +		-e 's!\@localstatedir\@!$(localstatedir)!g' \
>> +		-e 's|[@]libexecdir[@]|$(libexecdir)|g'     \
> Odd mix of \@ vs. [@].  For consistency, it might be nice to pick one
> form (and for that matter, \@ is non-portable sed) - but that's okay for
> a followup patch

Nah, I'd rather get it right the first time. (This was created by me
pasting in one bit from one location, then realizing I needed to replace
more variables and pasting in the 2nd bit from somewhere else).


> particularly since it is probably copy-and-paste from
> libvirt, meaning a similar patch is needed there too.

I just looked in all the libvirt Makefile.am's and didn't find any
occurences of \@ in a sed command. Probably those came from netcf's
Makefile.am.

Thanks for the review!



More information about the netcf-devel mailing list