On Thu, Dec 12, 2013 at 5:14 PM, Daniel P. Berrange <berrange@xxxxxxxxxx> wrote:
> On Tue, Nov 26, 2013 at 02:35:58AM +0530, Nehal J Wani wrote:
>> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
>> index 5aad75c..20ea40a 100644
>> --- a/include/libvirt/libvirt.h.in
>> +++ b/include/libvirt/libvirt.h.in
>> +
>> +typedef struct _virNetworkDHCPLeases virNetworkDHCPLeases;
>> +typedef virNetworkDHCPLeases *virNetworkDHCPLeasesPtr;
>> +struct _virNetworkDHCPLeases {
>
> s/Leases/Lease/ - each struct only stores a single lease
>
>> + char *interface; /* Network interface name */
>> + long long expirytime; /* Seconds since epoch */
>> + int type; /* virIPAddrType */
>> + char *mac; /* MAC address */
>> + char *iaid; /* IAID */
>> + char *ipaddr; /* IP address */
>> + unsigned int prefix; /* IP address prefix */
>> + char *hostname; /* Hostname */
>> + char *clientid; /* Client ID or DUID */
>> +};
>
>> @@ -2019,6 +2022,7 @@ libvirt_setuid_rpc_client_la_SOURCES = \
>> util/virtypedparam.c \
>> util/viruri.c \
>> util/virutil.c \
>> + util/virmacaddr.c \
>> util/viruuid.c \
>> conf/domain_event.c \
>> rpc/virnetsocket.c \
>
> Don't add this here.
The code didn't compile without making the above addition. It kept
throwing the error:
../src/.libs/libvirt-setuid-rpc-client.a(libvirt_setuid_rpc_client_la-libvirt.o):
In function `virNetworkGetDHCPLeasesForMAC':
/home/Wani/Hobby/libvirt/src/libvirt.c:22187: undefined reference to
`virMacAddrParse'
>
>> diff --git a/src/libvirt.c b/src/libvirt.c
>> index eff44eb..9a0b872 100644
>> --- a/src/libvirt.c
>> +++ b/src/libvirt.c
>> @@ -68,6 +68,7 @@
>> #include "virstring.h"
>> #include "virutil.h"
>> #include "virtypedparam.h"
>> +#include "virmacaddr.h"
>>
>> #ifdef WITH_TEST
>> # include "test/test_driver.h"
>
>> +int
>> +virNetworkGetDHCPLeasesForMAC(virNetworkPtr network,
>> + const char *mac,
>> + virNetworkDHCPLeasesPtr **leases,
>> + unsigned int flags)
>> +{
>> + virConnectPtr conn;
>> + virMacAddr addr;
>> +
>> + VIR_DEBUG("network=%p, mac=%s, leases=%p, flags=%x",
>> + network, mac, leases, flags);
>> +
>> + virResetLastError();
>> +
>> + if (leases)
>> + *leases = NULL;
>> +
>> + virCheckNonNullArgGoto(mac, error);
>> +
>> + if (!VIR_IS_CONNECTED_NETWORK(network)) {
>> + virLibNetworkError(VIR_ERR_INVALID_NETWORK, __FUNCTION__);
>> + virDispatchError(NULL);
>> + return -1;
>> + }
>> +
>> + /* Validate the MAC address */
>> + if (virMacAddrParse(mac, &addr) < 0) {
>> + virReportInvalidArg(mac, "%s",
>> + _("Given MAC Address doesn't comply "
>> + "with the standard (IEEE 802) format"));
>> + goto error;
>> + }
>
> Don't do this here - it is the job of driver APIs todo semantic
> validation of parameters.
Do you mean, I need to put this check inside
networkGetDHCPLeasesForMAC in src/network/bridge_driver.c ?
>
>
>> diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
>> index fe9b497..f1a9707 100644
>> --- a/src/libvirt_public.syms
>> +++ b/src/libvirt_public.syms
>> @@ -639,4 +639,11 @@ LIBVIRT_1.1.3 {
>> virConnectGetCPUModelNames;
>> } LIBVIRT_1.1.1;
>>
>> +LIBVIRT_1.1.4 {
>> + global:
>> + virNetworkDHCPLeaseFree;
>> + virNetworkGetDHCPLeases;
>> + virNetworkGetDHCPLeasesForMAC;
>> +} LIBVIRT_1.1.3;
>
> Can move this into the 1.2.1 version block now.
>
>> +/*
>> + * Use this when passing possibly-NULL strings to printf-a-likes.
>> + */
>> +# define NULL_STR(s) ((s) ? (s) : "*")
>
> I'd suggest calling this EMPTY_STR to avoid confusion with
> the existing NULLSTR macro which prints "(null)"
>
>> +
>> +int
>> +main(int argc, char **argv) {
>> +
>> + /* Doesn't hurt to check */
>> + if (argc < 4)
>> + return -1;
>
> You should print an error message to stderr along with
> usage text.
>
>> +
>> + const char *action = argv[1];
>> + const char *interface = NULL_STR(getenv("DNSMASQ_INTERFACE"));
>> + const char *expirytime = NULL_STR(getenv("DNSMASQ_LEASE_EXPIRES"));
>> + const char *mac = argv[2];
>> + const char *ip = argv[3];
>> + const char *iaid = NULL_STR(getenv("DNSMASQ_IAID"));
>> + const char *hostname = NULL_STR(getenv("DNSMASQ_SUPPLIED_HOSTNAME"));
>> + const char *clientid = NULL_STR(getenv("DNSMASQ_CLIENT_ID"));
>
> All use of getenv is banned in libvirt code - please make sure
> to run 'make syntax-check' and 'make check'. Use virGetEnvAllowSUID
> here
I did run both the commands. Unfortunately, they didn't detect that I
had been using getenv().
>
>> + const char *leases_str = NULL;
>> + char *lease_file = NULL;
>> + char *lease_entries = NULL;
>> + char *lease_entry = NULL;
>> + char **lease_fields = NULL;
>> + bool delete = false;
>> + bool add = false;
>> + int rv = -1;
>> + int lease_file_len = 0;
>> + FILE *fp = NULL;
>> + virBuffer buf_new_lease = VIR_BUFFER_INITIALIZER;
>> + virBuffer buf_all_leases = VIR_BUFFER_INITIALIZER;
>
> You must call the following before invoking any other libvir
> APIs at all
>
> if (setlocale(LC_ALL, "") == NULL ||
> bindtextdomain(PACKAGE, LOCALEDIR) == NULL ||
> textdomain(PACKAGE) == NULL) {
> fprintf(stderr, _("%s: initialization failed\n"), program_name);
> exit(EXIT_FAILURE);
> }
>
> if (virThreadInitialize() < 0 ||
> virErrorInitialize() < 0) {
> fprintf(stderr, _("%s: initialization failed\n"), program_name);
> exit(EXIT_FAILURE);
> }
>
>
Just a suggestion: It would be good if someone adds this to hacking.html
>> +
>> + if (virAsprintf(&lease_file, "%s/%s.status", LOCALSTATEDIR
>> + "/lib/libvirt/dnsmasq/", interface) < 0)
>> + goto cleanup;
>> +
>> + if (getenv("DNSMASQ_IAID")) {
>> + mac = NULL_STR(getenv("DNSMASQ_MAC"));
>> + clientid = argv[2];
>> + }
>> +
>> + /* Make sure the file exists. If not, 'touch' it */
>> + fp = fopen(lease_file, "a+");
>> + fclose(fp);
>
> We have a virFileTouch method todo this.
>
>> +
>> + /* Read entire contents */
>> + if ((lease_file_len = virFileReadAll(lease_file,
>> + VIR_NETWORK_DHCP_LEASE_FILE_SIZE_MAX,
>> + &lease_entries)) < 0) {
>> + goto cleanup;
>> + }
>> +
>> +
>> + if (STREQ(action, "add") || STREQ(action, "old") || STREQ(action, "del")) {
>> + if (mac || STREQ(action, "del")) {
>> + /* Delete the corresponding lease */
>> + delete = true;
>> + if (STREQ(action, "add") || STREQ(action, "old")) {
>> + add = true;
>> + /* Enter new lease */
>> + virBufferAsprintf(&buf_new_lease, "%s %s %s %s %s %s\n",
>> + expirytime, mac, iaid, ip, hostname, clientid);
>> +
>> + if (virBufferError(&buf_new_lease)) {
>> + virBufferFreeAndReset(&buf_new_lease);
>> + virReportOOMError();
>> + goto cleanup;
>> + }
>> + }
>> + }
>> + }
>> +
>> + lease_entry = lease_entries[0] == '\0' ? NULL : lease_entries;
>> +
>> + while (lease_entry) {
>> + int nfields = 0;
>> +
>> + char *eol = strchr(lease_entry, '\n');
>> + *eol = '\0';
>> +
>> + /* Split the lease line */
>> + if (!(lease_fields = virStringSplit(lease_entry, " ",
>> + VIR_NETWORK_DHCP_LEASE_FIELDS)))
>> + goto cleanup;
>> +
>> + nfields = virStringListLength(lease_fields);
>> +
>> + /* Forward lease_entry to the next lease */
>> + lease_entry = strchr(lease_entry, '\0');
>> + if (lease_entry - lease_entries + 1 < lease_file_len)
>> + lease_entry++;
>> + else
>> + lease_entry = NULL;
>> +
>> + if (nfields != VIR_NETWORK_DHCP_LEASE_FIELDS)
>> + goto cleanup;
>> +
>> + if (delete && STREQ(lease_fields[3], ip))
>> + continue;
>> + else {
>> + virBufferAsprintf(&buf_all_leases, "%s %s %s %s %s %s\n",
>> + lease_fields[0], lease_fields[1], lease_fields[2],
>> + lease_fields[3], lease_fields[4], lease_fields[5]);
>> +
>> + if (virBufferError(&buf_all_leases)) {
>> + virBufferFreeAndReset(&buf_all_leases);
>> + virReportOOMError();
>> + goto cleanup;
>> + }
>> + }
>> + }
>> +
>> + if (add)
>> + virBufferAsprintf(&buf_all_leases, "%s", virBufferContentAndReset(&buf_new_lease));
>> +
>
> Need virBufferError check on buf_all_leases here
>
>> + rv = 0;
>> +
>> + /* Write to file */
>> + leases_str = virBufferContentAndReset(&buf_all_leases);
>> + if (!leases_str)
>> + leases_str = "";
>> +
>> + if (virFileWriteStr(lease_file, leases_str, 0) < 0)
>> + rv = -1;
>> +
>> +cleanup:
>> + VIR_FREE(lease_file);
>> + VIR_FREE(lease_entries);
>> + if (lease_fields)
>> + virStringFreeList(lease_fields);
>> + return rv;
>> +}
>
>
> I'd suggest that the lease helper program should be added in a separate
> patch from the public API, sine there's no real dependancy between
> them.
Would do that in the next series.
Thanks for reviewing the series. I'll continue to furnish them.
>
> Daniel
> --
> |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org -o- http://virt-manager.org :|
> |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
> |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
Nehal J Wani
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list