[libvirt] [PATCH 2/5] network: change location of network state xml files

[Laine Stump]

On 04/15/2014 03:26 PM, John Ferlan wrote:
>
> On 04/10/2014 09:19 AM, Laine Stump wrote:
>> For some reason these have been stored in /var/lib, although other
>> drivers store their state files in /var/run.
>>
>> It's much nicer to store them in /var/run because they are
>> automatically cleared out when the system reboots. This can be a
>> useful way of learning whether or not a particular network is active.
>>
>> Note that this change by itself will cause problems in the case of an
>> upgrade from an older libvirt that uses /var/lib, and also in the case
>> of a downgrade from a newer libvirt using /var/run to an older version
>> using /var/lib. This compatibility problem will be addressed in the
>> next patch.
>> ---
>>  src/network/bridge_driver.c | 11 ++++++-----
>>  1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
>> index fdddc9a..a027b47 100644
>> --- a/src/network/bridge_driver.c
>> +++ b/src/network/bridge_driver.c
>> @@ -446,10 +446,11 @@ networkStateInitialize(bool privileged,
>>       * ~/.config/libvirt/... (session/unprivileged)
>>       * /etc/libvirt/... && /var/(run|lib)/libvirt/... (system/privileged).
>>       *
>> -     * NB: The qemu driver puts its domain state in /var/run, and I
>> -     * think the network driver should have used /var/run too (instead
>> -     * of /var/lib), but it's been this way for a long time, and we
>> -     * probably shouldn't change it now.
>> +     * NB: The network driver previously stored its status xml in
>> +     * /var/lib/libvirt/network instead of
>> +     * /var/run/libvirt/network. An upgrade downgrade between libvirt
>> +     * versions using different state directories may or may not cause
>> +     * problems.
>>       */
>>      if (privileged) {
>>          if (VIR_STRDUP(driverState->networkConfigDir,
>> @@ -457,7 +458,7 @@ networkStateInitialize(bool privileged,
>>              VIR_STRDUP(driverState->networkAutostartDir,
>>                         SYSCONFDIR "/libvirt/qemu/networks/autostart") < 0 ||
>>              VIR_STRDUP(driverState->stateDir,
>> -                       LOCALSTATEDIR "/lib/libvirt/network") < 0 ||
>> +                       LOCALSTATEDIR "/run/libvirt/network") < 0 ||
>>              VIR_STRDUP(driverState->pidDir,
>>                         LOCALSTATEDIR "/run/libvirt/network") < 0 ||
>>              VIR_STRDUP(driverState->dnsmasqStateDir,
>>
> This is only being done for the /var/... files and not the
> non-privileged case, right?

Correct.

>   I guess part of me wonders why not follow
> through and do the same in the else here

Because that else clause for non-privileged was added long after the
original code for privileged was written, and the directories are
already in the correct place from the beginning so they don't need to be
fixed.

>  - wouldn't that code have the
> same 'feature' you're trying to take advantage of regarding how the run
> directory is used?

I'm actually not sure exactly what is done with the directories in
~/.config (when/if the $run directory is cleared, etc), but there was
some sort of reconfig/standardization of the locations with gnome3 that
the qemu driver was made to follow, and the network driver copies those
locations faithfully (replacing "qemu" with "network" or "qemu/network"
as appropriate).



>   I suppose I assume that the non priv'd environment
> acts just like the priv'd one except for location.

non-privileged libvirtd is a bit odd. For starters it isn't run until
someone tries to connect to their user's qemu:///session, and then it
automatically exits after some timeout. Beyond that, the network driver
doesn't actually work (!!) for non-privileged libvirtd - adding the
~/.config directories was only done to prevent it from crashing, with
the thinking that if we *did* make it work sometime in the future, those
would be the right places to store the files (since it works for qemu...)


>
> Beyond that - it feels like 2 & 3 need to be combined or reworked to
> avoid the git bisect issues.

Well, yes and no. Both patches independently pass make check and make
syntax-check, and I hadn't considered that anyone would do a bisect
where the test at each step was to install an older libvirt, then
upgrade to the newly built bisect binary.

However, my main reason for separating the two was just to make review
easier (as well as in case I decided to make any more complicated
alternative to 3/5), and there's no point in dooming such a bisect test
to failure for no good reason, so I would be fine with merging 2/5 and 3/5.