Re: [libvirt] [PATCH v2] leaseshelper: improvements to support all events

2014-07-24 Thread Peter Krempa 
On 07/23/14 23:42, Nehal J Wani wrote:
 On IRC, we discussed another alternative - keep the top-level item as an
 array, and instead of adding server-duid as an array element, just add
 it as an optional field member of each {} ipv6 lease in the array
 (multiple copies of the string, but oh well).  At least that way, you
 aren't artificially adding a non-lease to the array itself, and
 hopefully libvirt 1.2.6 ignores unknown fields of a lease array entry.
 
 This seems to be the most easy option. Also, how about adding another
 field to every lease:
 generated-by: leaseshelper v$X (where X is the value provided by
 the macro PACKAGE_VERSION)
 so that whenever a new field is added in the future, we don't have
 trouble finding out which version of leaseshelper added it.

Hmmm, that doesn't seem to be such a good idea. If we want to add the
generated-by statement, we should wrap the leases array into an object.

Peter




signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2] leaseshelper: improvements to support all events

2014-07-23 Thread Eric Blake 
On 07/22/2014 09:20 AM, Peter Krempa wrote:
 On 07/22/14 01:03, Nehal J Wani wrote:
 This patch enables the helper program to detect event(s) triggered when there
 is a change in lease length or expiry and client-id. This transfers complete
 control of leases database to libvirt and obsoletes use of the lease database
 file (network-name.leases). That file will not be created, read, or 
 written.
 This is achieved by adding the option --leasefile-ro to dnsmasq and passing a
 custom env var to leaseshelper, which helps us map events related to leases
 with their corresponding network bridges, no matter what the event be.

 Also, this requires the addition of a new non-lease entry in our custom lease
 database: server-duid. It is required to identify a DHCPv6 server.

 Now that dnsmasq doesn't maintain its own leases database, it relies on our
 helper program to tell it about previous leases and server duid. Thus, this
 patch makes our leases program honor an extra action: init, in which it 
 sends
 the known info in a particular format to dnsmasq by printing it to stdout.

 ---
  This is compatible with libvirt 1.2.6 as only additions have been
  introduced, and the old leases file (*.staus) will still be supported.

s/staus/status/


 +else {
 +if (add) {
 +if (virJSONValueArrayAppend(leases_array_new, lease_new)  0) {
 +virReportError(VIR_ERR_INTERNAL_ERROR, %s,
 +   _(failed to create json));
 +goto cleanup;
 +}
 +lease_new = NULL;
 +}
 +
 +if (server_duid) {
 +if (!(lease_new = virJSONValueNewObject())) {
 +virReportError(VIR_ERR_INTERNAL_ERROR, %s,
 +   _(failed to create json));
 +goto cleanup;
 +}
 +
 +if (virJSONValueObjectAppendString(lease_new,
 +   server-duid, server_duid) 
  0)
 +goto cleanup;
 
 Hmm this is really odd. Why is the server_duid stored as a part of the
 leases array as it's a completely separate info and occuring only once?

Indeed.  The pre-patch file looks like:

[
{
iaid: 1221229,
ip-address: 2001:db8:ca2:2:1::95,
mac-address: 52:54:00:12:a2:6d,
hostname: Fedora20,
client-id:
00:04:1a:c1:d9:6b:5a:0a:e2:bc:f8:4b:1e:37:2e:38:22:55
expiry-time: 1393244216
},
...
]

I think the enhanced format should look like:

{
  server-duid: ...,
  leases: [
{
iaid: 1221229,
ip-address: 2001:db8:ca2:2:1::95,
mac-address: 52:54:00:12:a2:6d,
hostname: Fedora20,
client-id:
00:04:1a:c1:d9:6b:5a:0a:e2:bc:f8:4b:1e:37:2e:38:22:55
expiry-time: 1393244216
},
...
]
}

which means that when you first parse the file, the new code has to
determine if the parsed JSON item is an object (new style) or an array
(old style); and grab out the array from either style for the rest of
the code to manipulate.

 
 Unfortunately, looking at the format of the lease file the array of
 leases is the top level element. Is there a way we could (without
 complicating the code too much) convert it to a object so that it's
 extensible?

I agree - it's better to rearrange the file to place the new content as
a sibling to the existing content by creating another wrapper layer
around both, rather than commandeering an array slot for non-lease
information.

 
 The change to using the ENV variable has turned out great, unfortunately
 there's a problem with the lease file JSON we need to clear before
 proceeding.

Looking forward to v3.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2] leaseshelper: improvements to support all events

2014-07-23 Thread Eric Blake 
On 07/23/2014 02:32 PM, Eric Blake wrote:
 
 I think the enhanced format should look like:
 
 {
   server-duid: ...,
   leases: [
   {
 iaid: 1221229,
 ip-address: 2001:db8:ca2:2:1::95,
 mac-address: 52:54:00:12:a2:6d,
 hostname: Fedora20,
 client-id:
 00:04:1a:c1:d9:6b:5a:0a:e2:bc:f8:4b:1e:37:2e:38:22:55
 expiry-time: 1393244216
 },
 ...
 ]
 }

Another idea: make it easier for any future changes.  In your top-level
object, add some other fields:

{
  file-format: 1,
  comment: leases file generated by leaseshelper from libvirt 1.2.7,
  server-duid...

where file-format was implicitly 0 for the old style, and where you can
bump it to 2 if we have to make future changes; and where the comment is
free-form (human-readable, not machine-parseable) information that makes
it easier to debug what the file contains and which build of
leaseshelper generated it (since the leaseshelper version might be
different than libvirtd).


-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2] leaseshelper: improvements to support all events

2014-07-23 Thread Eric Blake 
On 07/23/2014 02:32 PM, Eric Blake wrote:
 I think the enhanced format should look like:
 
 {
   server-duid: ...,
   leases: [
   {
 iaid: 1221229,
 ip-address: 2001:db8:ca2:2:1::95,
 mac-address: 52:54:00:12:a2:6d,
 hostname: Fedora20,
 client-id:
 00:04:1a:c1:d9:6b:5a:0a:e2:bc:f8:4b:1e:37:2e:38:22:55
 expiry-time: 1393244216
 },
 ...
 ]

On IRC, we discussed another alternative - keep the top-level item as an
array, and instead of adding server-duid as an array element, just add
it as an optional field member of each {} ipv6 lease in the array
(multiple copies of the string, but oh well).  At least that way, you
aren't artificially adding a non-lease to the array itself, and
hopefully libvirt 1.2.6 ignores unknown fields of a lease array entry.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2] leaseshelper: improvements to support all events

2014-07-23 Thread Nehal J Wani 
 On IRC, we discussed another alternative - keep the top-level item as an
 array, and instead of adding server-duid as an array element, just add
 it as an optional field member of each {} ipv6 lease in the array
 (multiple copies of the string, but oh well).  At least that way, you
 aren't artificially adding a non-lease to the array itself, and
 hopefully libvirt 1.2.6 ignores unknown fields of a lease array entry.

This seems to be the most easy option. Also, how about adding another
field to every lease:
generated-by: leaseshelper v$X (where X is the value provided by
the macro PACKAGE_VERSION)
so that whenever a new field is added in the future, we don't have
trouble finding out which version of leaseshelper added it.


Regards,
Nehal J Wani

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2] leaseshelper: improvements to support all events

2014-07-22 Thread Peter Krempa 
On 07/22/14 01:03, Nehal J Wani wrote:
 This patch enables the helper program to detect event(s) triggered when there
 is a change in lease length or expiry and client-id. This transfers complete
 control of leases database to libvirt and obsoletes use of the lease database
 file (network-name.leases). That file will not be created, read, or written.
 This is achieved by adding the option --leasefile-ro to dnsmasq and passing a
 custom env var to leaseshelper, which helps us map events related to leases
 with their corresponding network bridges, no matter what the event be.
 
 Also, this requires the addition of a new non-lease entry in our custom lease
 database: server-duid. It is required to identify a DHCPv6 server.
 
 Now that dnsmasq doesn't maintain its own leases database, it relies on our
 helper program to tell it about previous leases and server duid. Thus, this
 patch makes our leases program honor an extra action: init, in which it 
 sends
 the known info in a particular format to dnsmasq by printing it to stdout.
 
 ---
  This is compatible with libvirt 1.2.6 as only additions have been
  introduced, and the old leases file (*.staus) will still be supported.
 
  v1: https://www.redhat.com/archives/libvir-list/2014-July/msg00568.html
 
  src/network/bridge_driver.c |   7 ++
  src/network/leaseshelper.c  | 152 
 +---
  2 files changed, 136 insertions(+), 23 deletions(-)
 
 diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
 index 6a2e760..4363cd8 100644
 --- a/src/network/bridge_driver.c
 +++ b/src/network/bridge_driver.c
 @@ -1289,7 +1289,10 @@ networkBuildDhcpDaemonCommandLine(virNetworkObjPtr 
 network,
  
  cmd = virCommandNew(dnsmasqCapsGetBinaryPath(caps));
  virCommandAddArgFormat(cmd, --conf-file=%s, configfile);
 +/* Libvirt gains full control of leases database */
 +virCommandAddArgFormat(cmd, --leasefile-ro);
  virCommandAddArgFormat(cmd, --dhcp-script=%s, leaseshelper_path);
 +virCommandAddEnvPair(cmd, VIR_BRIDGE_NAME, network-def-bridge);

The invocation is now much nicer!

  
  *cmdout = cmd;
  ret = 0;
 @@ -3432,6 +3435,10 @@ networkGetDHCPLeases(virNetworkPtr network,
  goto error;
  }
  
 +/* Ignore server-duid. It's not part of a lease */
 +if (virJSONValueObjectHasKey(lease_tmp, server-duid))
 +continue;

[1]

 +
  if (!(mac_tmp = virJSONValueObjectGetString(lease_tmp, 
 mac-address))) {
  /* leaseshelper program guarantees that lease will be stored 
 only if
   * mac-address is known otherwise not */
 diff --git a/src/network/leaseshelper.c b/src/network/leaseshelper.c
 index e4b5283..cc4b4ac 100644
 --- a/src/network/leaseshelper.c
 +++ b/src/network/leaseshelper.c

 @@ -89,6 +95,7 @@ enum virLeaseActionFlags {
  VIR_LEASE_ACTION_ADD,   /* Create new lease */
  VIR_LEASE_ACTION_OLD,   /* Lease already exists, renew it */
  VIR_LEASE_ACTION_DEL,   /* Delete the lease */
 +VIR_LEASE_ACTION_INIT,  /* Tell dnsmasq of existing leases on 
 restart */
  
  VIR_LEASE_ACTION_LAST
  };

 @@ -105,6 +112,7 @@ main(int argc, char **argv)
  char *pid_file = NULL;
  char *lease_entries = NULL;
  char *custom_lease_file = NULL;
 +char *server_duid = NULL;
  const char *ip = NULL;
  const char *mac = NULL;
  const char *iaid = virGetEnvAllowSUID(DNSMASQ_IAID);
 @@ -112,20 +120,26 @@ main(int argc, char **argv)
  const char *interface = virGetEnvAllowSUID(DNSMASQ_INTERFACE);
  const char *exptime_tmp = virGetEnvAllowSUID(DNSMASQ_LEASE_EXPIRES);
  const char *hostname = virGetEnvAllowSUID(DNSMASQ_SUPPLIED_HOSTNAME);
 +const char *server_duid_env = virGetEnvAllowSUID(DNSMASQ_SERVER_DUID);
  const char *leases_str = NULL;
  long long currtime = 0;
  long long expirytime = 0;
  size_t i = 0;
 +size_t count_ipv6 = 0;
 +size_t count_ipv4 = 0;
  int action = -1;
  int pid_file_fd = -1;
  int rv = EXIT_FAILURE;
  int custom_lease_file_len = 0;
  bool add = false;
 +bool init = false;
  bool delete = false;
  virJSONValuePtr lease_new = NULL;
  virJSONValuePtr lease_tmp = NULL;
  virJSONValuePtr leases_array = NULL;
  virJSONValuePtr leases_array_new = NULL;
 +virJSONValuePtr *leases_ipv4 = NULL;
 +virJSONValuePtr *leases_ipv6 = NULL;
  
  virSetErrorFunc(NULL, NULL);
  virSetErrorLogPriorityFunc(NULL);
 @@ -156,17 +170,18 @@ main(int argc, char **argv)
  }
  }
  
 -if (argc != 4  argc != 5) {
 +if (argc != 4  argc != 5  argc != 2) {
  /* Refer man page of dnsmasq --dhcp-script for more details */
  usage(EXIT_FAILURE);
  }
  
  /* Make sure dnsmasq knows the interface. The interface name is not known
 - * when dnsmasq (re)starts and throws 'del' events for expired leases.
 - * So, if any old lease has expired, it will

[libvirt] [PATCH v2] leaseshelper: improvements to support all events

2014-07-21 Thread Nehal J Wani 
This patch enables the helper program to detect event(s) triggered when there
is a change in lease length or expiry and client-id. This transfers complete
control of leases database to libvirt and obsoletes use of the lease database
file (network-name.leases). That file will not be created, read, or written.
This is achieved by adding the option --leasefile-ro to dnsmasq and passing a
custom env var to leaseshelper, which helps us map events related to leases
with their corresponding network bridges, no matter what the event be.

Also, this requires the addition of a new non-lease entry in our custom lease
database: server-duid. It is required to identify a DHCPv6 server.

Now that dnsmasq doesn't maintain its own leases database, it relies on our
helper program to tell it about previous leases and server duid. Thus, this
patch makes our leases program honor an extra action: init, in which it sends
the known info in a particular format to dnsmasq by printing it to stdout.

---
 This is compatible with libvirt 1.2.6 as only additions have been
 introduced, and the old leases file (*.staus) will still be supported.

 v1: https://www.redhat.com/archives/libvir-list/2014-July/msg00568.html

 src/network/bridge_driver.c |   7 ++
 src/network/leaseshelper.c  | 152 +---
 2 files changed, 136 insertions(+), 23 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 6a2e760..4363cd8 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -1289,7 +1289,10 @@ networkBuildDhcpDaemonCommandLine(virNetworkObjPtr 
network,
 
 cmd = virCommandNew(dnsmasqCapsGetBinaryPath(caps));
 virCommandAddArgFormat(cmd, --conf-file=%s, configfile);
+/* Libvirt gains full control of leases database */
+virCommandAddArgFormat(cmd, --leasefile-ro);
 virCommandAddArgFormat(cmd, --dhcp-script=%s, leaseshelper_path);
+virCommandAddEnvPair(cmd, VIR_BRIDGE_NAME, network-def-bridge);
 
 *cmdout = cmd;
 ret = 0;
@@ -3432,6 +3435,10 @@ networkGetDHCPLeases(virNetworkPtr network,
 goto error;
 }
 
+/* Ignore server-duid. It's not part of a lease */
+if (virJSONValueObjectHasKey(lease_tmp, server-duid))
+continue;
+
 if (!(mac_tmp = virJSONValueObjectGetString(lease_tmp, 
mac-address))) {
 /* leaseshelper program guarantees that lease will be stored only 
if
  * mac-address is known otherwise not */
diff --git a/src/network/leaseshelper.c b/src/network/leaseshelper.c
index e4b5283..cc4b4ac 100644
--- a/src/network/leaseshelper.c
+++ b/src/network/leaseshelper.c
@@ -50,6 +50,12 @@
  */
 #define VIR_NETWORK_DHCP_LEASE_FILE_SIZE_MAX (32 * 1024 * 1024)
 
+/*
+ * Use this when passing possibly-NULL strings to printf-a-likes.
+ * Required for unknown parameters during init call.
+ */
+#define EMPTY_STR(s) ((s) ? (s) : *)
+
 static const char *program_name;
 
 /* Display version information. */
@@ -65,7 +71,7 @@ usage(int status)
 if (status) {
 fprintf(stderr, _(%s: try --help for more details\n), program_name);
 } else {
-printf(_(Usage: %s add|old|del mac|clientid ip [hostname]\n
+printf(_(Usage: %s add|old|del|init mac|clientid ip [hostname]\n
  Designed for use with 'dnsmasq --dhcp-script'\n
  Refer to man page of dnsmasq for more details'\n),
program_name);
@@ -89,6 +95,7 @@ enum virLeaseActionFlags {
 VIR_LEASE_ACTION_ADD,   /* Create new lease */
 VIR_LEASE_ACTION_OLD,   /* Lease already exists, renew it */
 VIR_LEASE_ACTION_DEL,   /* Delete the lease */
+VIR_LEASE_ACTION_INIT,  /* Tell dnsmasq of existing leases on restart 
*/
 
 VIR_LEASE_ACTION_LAST
 };
@@ -96,7 +103,7 @@ enum virLeaseActionFlags {
 VIR_ENUM_DECL(virLeaseAction);
 
 VIR_ENUM_IMPL(virLeaseAction, VIR_LEASE_ACTION_LAST,
-  add, old, del);
+  add, old, del, init);
 
 int
 main(int argc, char **argv)
@@ -105,6 +112,7 @@ main(int argc, char **argv)
 char *pid_file = NULL;
 char *lease_entries = NULL;
 char *custom_lease_file = NULL;
+char *server_duid = NULL;
 const char *ip = NULL;
 const char *mac = NULL;
 const char *iaid = virGetEnvAllowSUID(DNSMASQ_IAID);
@@ -112,20 +120,26 @@ main(int argc, char **argv)
 const char *interface = virGetEnvAllowSUID(DNSMASQ_INTERFACE);
 const char *exptime_tmp = virGetEnvAllowSUID(DNSMASQ_LEASE_EXPIRES);
 const char *hostname = virGetEnvAllowSUID(DNSMASQ_SUPPLIED_HOSTNAME);
+const char *server_duid_env = virGetEnvAllowSUID(DNSMASQ_SERVER_DUID);
 const char *leases_str = NULL;
 long long currtime = 0;
 long long expirytime = 0;
 size_t i = 0;
+size_t count_ipv6 = 0;
+size_t count_ipv4 = 0;
 int action = -1;
 int pid_file_fd = -1;
 int rv = EXIT_FAILURE;
 int custom_lease_file_len = 0;
 bool add = false;
+bool