Re: [PATCHv5 1/4] net-dhcp-leases: Implement the public APIs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]