[PATCH 0 of 4] Update KVMRedirectionSAP to return avail sessions
This updated uses the listening / established port entries in /proc/net/tcp to determine whether a guest has any available / enabled VNC sessions.
# HG changeset patch # User kaitlin@elm3b43.beaverton.ibm.com # Date 1224552268 25200 # Node ID 155077ac9e3ca778dc8564aa86e991f24f373e7e # Parent f0a209b602e707305a713611097310ec503451df Add vnc_ports struct and functions for building a list of ports. Signed-off-by: Kaitlin Rupert <karupert@us.ibm.com> diff -r f0a209b602e7 -r 155077ac9e3c src/Virt_KVMRedirectionSAP.c --- a/src/Virt_KVMRedirectionSAP.c Wed Oct 08 09:13:55 2008 -0700 +++ b/src/Virt_KVMRedirectionSAP.c Mon Oct 20 18:24:28 2008 -0700 @@ -39,7 +39,75 @@ #include "Virt_KVMRedirectionSAP.h" +#define PROC_TCP "/proc/net/tcp" +#define VNC_PORT_MIN 5900 +#define VNC_PORT_MAX 5999 +#define HASH_SIZE 128 + const static CMPIBroker *_BROKER; + +struct vnc_ports { + char **list; + unsigned int max; + unsigned int cur; +}; + +static int resize(struct vnc_ports *list, int newmax) +{ + char **newlist; + int i; + + newlist = realloc(list->list, newmax * sizeof(char *)); + if (!newlist) + return 0; + + list->max = newmax; + list->list = newlist; + + for (i = list->cur; i < list->max; i++) + list->list[i] = NULL; + + return 1; +} + +static void vnc_list_init(struct vnc_ports *vnc_hash[]) +{ + int i; + + for (i = 0; i < HASH_SIZE; i++) { + vnc_hash[i] = malloc(sizeof(struct vnc_ports)); + vnc_hash[i]->list = NULL; + vnc_hash[i]->cur = vnc_hash[i]->max = 0; + } +} + +static void vnc_list_free(struct vnc_ports *vnc_hash[]) +{ + int i; + + for (i = 0; i < HASH_SIZE; i++) { + if (vnc_hash[i]->list != NULL) + free(vnc_hash[i]->list); + } + vnc_list_init(vnc_hash); +} + +static int vnc_list_add(struct vnc_ports *list, char *port) +{ + if ((list->cur + 1) >= list->max) { + int ret; + + ret = resize(list, list->max + 5); + + if (!ret) + return 0; + } + + list->list[list->cur] = strdup(port); + list->cur++; + + return 1; +} static int inst_from_dom(const CMPIBroker *broker, const CMPIObjectPath *ref,
KR> +#define VNC_PORT_MIN 5900 KR> +#define VNC_PORT_MAX 5999 KR> +#define HASH_SIZE 128 Your hash size is larger than your key domain. That means that unless your hash function is really bad, you should never have any collisions... :) The domain is small enough, that I'd just use an array of MAX-MIN, with a hash function of modulus. KR> +static int resize(struct vnc_ports *list, int newmax) KR> +{ KR> + char **newlist; KR> + int i; KR> + KR> + newlist = realloc(list->list, newmax * sizeof(char *)); KR> + if (!newlist) This should be: if (newlist != NULL) KR> +static void vnc_list_init(struct vnc_ports *vnc_hash[]) KR> +{ KR> + int i; KR> + KR> + for (i = 0; i < HASH_SIZE; i++) { KR> + vnc_hash[i] = malloc(sizeof(struct vnc_ports)); KR> + vnc_hash[i]->list = NULL; Crash here if the above malloc() fails. KR> + vnc_hash[i]->cur = vnc_hash[i]->max = 0; KR> + } KR> +} Since this function can fail, it needs to be non-void and return an indication of success. Alternately, since the array of buckets is static anyway, why not just use a static array instead of allocating memory for all the buckets? KR> +static void vnc_list_free(struct vnc_ports *vnc_hash[]) KR> +{ KR> + int i; KR> + KR> + for (i = 0; i < HASH_SIZE; i++) { You've got a whitespace issue here. KR> + if (vnc_hash[i]->list != NULL) KR> + free(vnc_hash[i]->list); KR> + } You free the list, but you don't free the pointers in the list. Since you're loading those up with the result of strdup() in vnc_list_add(), you leak all of that memory. KR> + vnc_list_init(vnc_hash); This is in vnc_list_free(), but doesn't vnc_list_init() allocate memory for the buckets? This seems to go through and free the list member of each bucket, and then leak the memory for the actual bucket by allocating more memory over top of them. KR> +static int vnc_list_add(struct vnc_ports *list, char *port) KR> +{ KR> + if ((list->cur + 1) >= list->max) { KR> + int ret; KR> + KR> + ret = resize(list, list->max + 5); KR> + KR> + if (!ret) KR> + return 0; KR> + } KR> + KR> + list->list[list->cur] = strdup(port); KR> + list->cur++; KR> + KR> + return 1; KR> +} Any reason not to just use a linked list? Since we're not expecting a lot of ports (100 maximum per system, according to your limits), doing this whole resize thing seems kinda overkill. Inserting at the head if a linked list is easy and cheap. Better yet, why not just keep a count of sessions and avoid all the chaining, collision resolution, and allocation madness? :) -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
# HG changeset patch # User kaitlin@elm3b43.beaverton.ibm.com # Date 1224552270 25200 # Node ID 6d537f5e2b65c5d04963e9ff4e44e74c1e22b494 # Parent 155077ac9e3ca778dc8564aa86e991f24f373e7e Add get_vnc_sessions() to determine all of the VNC ports available or in use. Create a hash table where the port_value % 5900 is the has key. Signed-off-by: Kaitlin Rupert <karupert@us.ibm.com> diff -r 155077ac9e3c -r 6d537f5e2b65 src/Virt_KVMRedirectionSAP.c --- a/src/Virt_KVMRedirectionSAP.c Mon Oct 20 18:24:28 2008 -0700 +++ b/src/Virt_KVMRedirectionSAP.c Mon Oct 20 18:24:30 2008 -0700 @@ -109,6 +109,52 @@ return 1; } +static size_t get_vnc_hash_key(unsigned int key) +{ + return key % VNC_PORT_MIN; +} + +static CMPIStatus port_to_str(unsigned int port, + char **port_str) +{ + CMPIStatus s = {CMPI_RC_OK, NULL}; + + if (asprintf(port_str, "%" PRIu16, port) == -1) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Unable to determine session port"); + } + + return s; +} + +static CMPIStatus port_convert(unsigned int port, + char *in_str, + int *out_port) +{ + CMPIStatus s = {CMPI_RC_OK, NULL}; + char *str = NULL; + + if (in_str == NULL) { + s = port_to_str(port, &str); + if (s.rc != CMPI_RC_OK) + goto out; + } else + str = strdup(in_str); + + *out_port = strtol(str, NULL, 0); + if ((*out_port == LONG_MIN) || (*out_port == LONG_MAX)) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Failed to get port value"); + } + + out: + free(str); + + return s; +} + static int inst_from_dom(const CMPIBroker *broker, const CMPIObjectPath *ref, struct domain *dominfo, @@ -197,6 +243,68 @@ return inst; } +static CMPIStatus get_vnc_sessions(struct vnc_ports *vnc_hash[]) +{ + CMPIStatus s = {CMPI_RC_OK, NULL}; + const char *path = PROC_TCP; + unsigned int lport = 0; + unsigned int rport = 0; + FILE *tcp_info; + char *line = NULL; + size_t len = 0; + char *remote_port; + int local_port; + int index; + int val; + int ret; + + tcp_info = fopen(path, "r"); + if (tcp_info== NULL) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Failed to open %s: %m", tcp_info); + goto out; + } + + if (getline(&line, &len, tcp_info) == -1) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Failed to read from %s", tcp_info); + goto out; + } + + while (getline(&line, &len, tcp_info) > 0) { + ret = sscanf(line, "%d: %*[^:]:%X %*[^:]:%X", &val, &lport, + &rport); + if (ret != 3) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Unable to determine active sessions"); + goto out; + } + + s = port_convert(lport, NULL, &local_port); + if (s.rc != CMPI_RC_OK) + goto out; + + if ((local_port < VNC_PORT_MIN) || (local_port > VNC_PORT_MAX)) + continue; + + s = port_to_str(rport, &remote_port); + if (s.rc != CMPI_RC_OK) + goto out; + + index = get_vnc_hash_key(local_port); + + vnc_list_add(vnc_hash[index], remote_port); + free(remote_port); + } + + out: + fclose(tcp_info); + return s; +} + static bool check_graphics(virDomainPtr dom, struct domain **dominfo) { @@ -225,6 +333,7 @@ virDomainPtr *domain_list; struct domain *dominfo = NULL; struct inst_list list; + struct vnc_ports* vnc_hash[HASH_SIZE]; int count; int i; @@ -233,6 +342,7 @@ return s; inst_list_init(&list); + vnc_list_init(vnc_hash); count = get_domain_list(conn, &domain_list); if (count < 0) { @@ -242,6 +352,10 @@ goto out; } else if (count == 0) goto out; + + s = get_vnc_sessions(vnc_hash); + if (s.rc != CMPI_RC_OK) + goto out; for (i = 0; i < count; i++) { CMPIInstance *inst = NULL; @@ -269,6 +383,7 @@ out: free(domain_list); inst_list_free(&list); + vnc_list_free(vnc_hash); return s; }
KR> +static size_t get_vnc_hash_key(unsigned int key) KR> +{ KR> + return key % VNC_PORT_MIN; KR> +} What happens if I set my guest to VNC port 500? KR> +static CMPIStatus port_to_str(unsigned int port, KR> + char **port_str) KR> +{ KR> + CMPIStatus s = {CMPI_RC_OK, NULL}; KR> + KR> + if (asprintf(port_str, "%" PRIu16, port) == -1) { KR> + cu_statusf(_BROKER, &s, KR> + CMPI_RC_ERR_FAILED, KR> + "Unable to determine session port"); KR> + } KR> + KR> + return s; KR> +} Hmm, I don't like the idea of alloc'ing strings to store ports. They're just integers after all :) KR> +static CMPIStatus port_convert(unsigned int port, KR> + char *in_str, KR> + int *out_port) KR> +{ KR> + CMPIStatus s = {CMPI_RC_OK, NULL}; KR> + char *str = NULL; KR> + KR> + if (in_str == NULL) { KR> + s = port_to_str(port, &str); KR> + if (s.rc != CMPI_RC_OK) KR> + goto out; KR> + } else KR> + str = strdup(in_str); KR> + KR> + *out_port = strtol(str, NULL, 0); I think sscanf() is always a better choice. KR> +static CMPIStatus get_vnc_sessions(struct vnc_ports *vnc_hash[]) KR> +{ KR> + CMPIStatus s = {CMPI_RC_OK, NULL}; KR> + const char *path = PROC_TCP; KR> + unsigned int lport = 0; KR> + unsigned int rport = 0; KR> + FILE *tcp_info; KR> + char *line = NULL; KR> + size_t len = 0; KR> + char *remote_port; KR> + int local_port; KR> + int index; KR> + int val; KR> + int ret; KR> + KR> + tcp_info = fopen(path, "r"); KR> + if (tcp_info== NULL) { KR> + cu_statusf(_BROKER, &s, KR> + CMPI_RC_ERR_FAILED, KR> + "Failed to open %s: %m", tcp_info); KR> + goto out; KR> + } KR> + KR> + if (getline(&line, &len, tcp_info) == -1) { KR> + cu_statusf(_BROKER, &s, KR> + CMPI_RC_ERR_FAILED, KR> + "Failed to read from %s", tcp_info); KR> + goto out; KR> + } KR> + KR> + while (getline(&line, &len, tcp_info) > 0) { KR> + ret = sscanf(line, "%d: %*[^:]:%X %*[^:]:%X", &val, &lport, KR> + &rport); KR> + if (ret != 3) { KR> + cu_statusf(_BROKER, &s, KR> + CMPI_RC_ERR_FAILED, KR> + "Unable to determine active sessions"); KR> + goto out; KR> + } KR> + KR> + s = port_convert(lport, NULL, &local_port); KR> + if (s.rc != CMPI_RC_OK) KR> + goto out; KR> + KR> + if ((local_port < VNC_PORT_MIN) || (local_port > VNC_PORT_MAX)) KR> + continue; Ah, this is good that you check the range to avoid the crash :) However, do we want to support guests with non-standard ports? Seems like we do, so maybe we do need some collision resolution in the hash logic after all. -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
# HG changeset patch # User kaitlin@elm3b43.beaverton.ibm.com # Date 1224553244 25200 # Node ID d62634a3121f2991ed800491b2966ed9b531d4c3 # Parent 6d537f5e2b65c5d04963e9ff4e44e74c1e22b494 Return active/available/inactive console session Instead of one session per guest. Changes: -Change Name attribute so that is is formatted in the following way: "guest_name/guest_port:remote_port" -Check /proc/net/tcp for listening / enabled ports Signed-off-by: Kaitlin Rupert <karupert@us.ibm.com> diff -r 6d537f5e2b65 -r d62634a3121f src/Virt_KVMRedirectionSAP.c --- a/src/Virt_KVMRedirectionSAP.c Mon Oct 20 18:24:30 2008 -0700 +++ b/src/Virt_KVMRedirectionSAP.c Mon Oct 20 18:40:44 2008 -0700 @@ -23,6 +23,7 @@ #include <stdlib.h> #include <stdbool.h> #include <inttypes.h> +#include <limits.h> #include "cmpidt.h" #include "cmpift.h" @@ -155,21 +156,25 @@ return s; } -static int inst_from_dom(const CMPIBroker *broker, - const CMPIObjectPath *ref, - struct domain *dominfo, - CMPIInstance *inst) +static CMPIStatus inst_from_dom(const CMPIBroker *broker, + const CMPIObjectPath *ref, + struct domain *dominfo, + char *remote_port, + CMPIInstance *inst) { + CMPIStatus s = {CMPI_RC_OK, NULL}; char *sccn = NULL; char *id = NULL; char *pfx = NULL; uint16_t prop_val; - int ret = 1; + int val; - if (asprintf(&id, "%s:%s", dominfo->name, - dominfo->dev_graphics->dev.graphics.type) == -1) { - CU_DEBUG("Unable to format name"); - ret = 0; + if (asprintf(&id, "%s/%s:%s", dominfo->name, + dominfo->dev_graphics->dev.graphics.port, + remote_port) == -1) { + cu_statusf(broker, &s, + CMPI_RC_ERR_FAILED, + "Unable to format instance name"); goto out; } @@ -196,11 +201,22 @@ CMSetProperty(inst, "KVMProtocol", (CMPIValue *)&prop_val, CMPI_uint16); - /* Need to replace this with a check that determines whether - the console session is enabled (in use) or available (not actively - in use). - */ - prop_val = (uint16_t)CIM_CRS_ENABLED_STATE; + s = port_convert(0, dominfo->dev_graphics->dev.graphics.port, &val); + if (s.rc != CMPI_RC_OK) { + cu_statusf(broker, &s, + CMPI_RC_ERR_FAILED, + "Unable to format session port"); + goto out; + } + + /* If port is < 0, the session is inactive. */ + if (val < 0) + prop_val = (uint16_t)CIM_SAP_INACTIVE_STATE; + else if (STREQ(remote_port, "0")) + prop_val = (uint16_t)CIM_SAP_AVAILABLE_STATE; + else + prop_val = (uint16_t)CIM_SAP_ACTIVE_STATE; + CMSetProperty(inst, "EnabledState", (CMPIValue *)&prop_val, CMPI_uint16); @@ -209,13 +225,14 @@ free(id); free(sccn); - return ret; + return s; } static CMPIInstance *get_console_sap(const CMPIBroker *broker, const CMPIObjectPath *reference, virConnectPtr conn, struct domain *dominfo, + char *remote_port, CMPIStatus *s) { @@ -233,14 +250,68 @@ goto out; } - if (inst_from_dom(broker, reference, dominfo, inst) != 1) { - cu_statusf(broker, s, - CMPI_RC_ERR_FAILED, - "Unable to get instance from domain"); - } + *s = inst_from_dom(broker, reference, dominfo, remote_port, inst); out: return inst; +} + +static CMPIStatus get_session_insts(const CMPIObjectPath *ref, + virConnectPtr conn, + struct domain *dominfo, + struct vnc_ports *vnc_hash[], + struct inst_list *list) +{ + CMPIStatus s = {CMPI_RC_OK, NULL}; + CMPIInstance *inst; + char *dport = NULL; + int dport_int = 0; + int index; + int i; + + dport = strdup(dominfo->dev_graphics->dev.graphics.port); + + s = port_convert(0, dport, &dport_int); + if (s.rc != CMPI_RC_OK) + goto out; + + /* If port is < 0, the session is inactive. No need to check + for active / available connections. */ + if (dport_int < 0) { + inst = get_console_sap(_BROKER, ref, conn, dominfo, "-1", &s); + if (s.rc != CMPI_RC_OK) + goto out; + + if (inst != NULL) + inst_list_add(list, inst); + + goto out; + } + + /* Otherwise, check for active / available sessions. */ + index = get_vnc_hash_key(dport_int); + if (vnc_hash[index]->list != NULL) { + for (i = 0; i < vnc_hash[index]->cur; i++) { + inst = get_console_sap(_BROKER, ref, conn, dominfo, + vnc_hash[index]->list[i], &s); + if (s.rc != CMPI_RC_OK) + goto out; + + if (inst != NULL) + inst_list_add(list, inst); + } + } else { + inst = get_console_sap(_BROKER, ref, conn, dominfo, "-1", &s); + if (s.rc != CMPI_RC_OK) + goto out; + + if (inst != NULL) + inst_list_add(list, inst); + } + out: + free(dport); + + return s; } static CMPIStatus get_vnc_sessions(struct vnc_ports *vnc_hash[]) @@ -358,21 +429,19 @@ goto out; for (i = 0; i < count; i++) { - CMPIInstance *inst = NULL; - if (!check_graphics(domain_list[i], &dominfo)) { virDomainFree(domain_list[i]); cleanup_dominfo(&dominfo); continue; } - inst = get_console_sap(_BROKER, ref, conn, dominfo, &s); - virDomainFree(domain_list[i]); + + s = get_session_insts(ref, conn, dominfo, vnc_hash, &list); cleanup_dominfo(&dominfo); - if (inst != NULL) - inst_list_add(&list, inst); + if (s.rc != CMPI_RC_OK) + goto out; } if (names_only) diff -r 6d537f5e2b65 -r d62634a3121f src/svpc_types.h --- a/src/svpc_types.h Mon Oct 20 18:24:30 2008 -0700 +++ b/src/svpc_types.h Mon Oct 20 18:40:44 2008 -0700 @@ -59,6 +59,10 @@ #define CIM_CRS_OTHER 1 #define CIM_CRS_VNC 4 +#define CIM_SAP_ACTIVE_STATE 2 +#define CIM_SAP_INACTIVE_STATE 3 +#define CIM_SAP_AVAILABLE_STATE 6 + #include <libcmpiutil/libcmpiutil.h> #include <string.h>
# HG changeset patch # User kaitlin@elm3b43.beaverton.ibm.com # Date 1224553245 25200 # Node ID c41bd7c3175a970f7c674931ee27ef2a88b63b61 # Parent d62634a3121f2991ed800491b2966ed9b531d4c3 Have get_console_sap_by_name() parse Name attribute to determine session status Signed-off-by: Kaitlin Rupert <karupert@us.ibm.com> diff -r d62634a3121f -r c41bd7c3175a src/Virt_KVMRedirectionSAP.c --- a/src/Virt_KVMRedirectionSAP.c Mon Oct 20 18:40:44 2008 -0700 +++ b/src/Virt_KVMRedirectionSAP.c Mon Oct 20 18:40:45 2008 -0700 @@ -113,6 +113,33 @@ static size_t get_vnc_hash_key(unsigned int key) { return key % VNC_PORT_MIN; +} + +static int parse_sap_name(const char *name, + char **guest, + char **port, + char **remote_port) +{ + int ret; + + ret = sscanf(name, "%a[^/]/%a[^:]:%as", + guest, + port, + remote_port); + + if (ret != 3) { + free(*guest); + free(*port); + free(*remote_port); + + *guest = NULL; + *port = NULL; + *remote_port = NULL; + + return 0; + } + + return 1; } static CMPIStatus port_to_str(unsigned int port, @@ -459,14 +486,18 @@ CMPIStatus get_console_sap_by_name(const CMPIBroker *broker, const CMPIObjectPath *ref, - const char *name, + const char *id, CMPIInstance **_inst) { virConnectPtr conn; - virDomainPtr dom; + virDomainPtr dom = NULL; CMPIStatus s = {CMPI_RC_OK, NULL}; CMPIInstance *inst = NULL; struct domain *dominfo = NULL; + char *name = NULL; + char *port = NULL; + char *remote_port = NULL; + int ret; conn = connect_by_classname(broker, CLASSNAME(ref), &s); if (conn == NULL) { @@ -476,6 +507,14 @@ goto out; } + ret = parse_sap_name(id, &name, &port, &remote_port); + if (ret != 1) { + cu_statusf(broker, &s, + CMPI_RC_ERR_FAILED, + "Unable to parse instance name"); + goto out; + } + dom = virDomainLookupByName(conn, name); if (dom == NULL) { cu_statusf(broker, &s, @@ -489,19 +528,21 @@ cu_statusf(broker, &s, CMPI_RC_ERR_FAILED, "No console device for this guest"); + + goto out; } - inst = get_console_sap(_BROKER, ref, conn, dominfo, &s); - - virDomainFree(dom); - - if (s.rc != CMPI_RC_OK) - goto out; + inst = get_console_sap(_BROKER, ref, conn, dominfo, remote_port, &s); *_inst = inst; out: + virDomainFree(dom); virConnectClose(conn); + + free(name); + free(port); + free(remote_port); return s; } @@ -512,16 +553,16 @@ { CMPIStatus s = {CMPI_RC_OK, NULL}; CMPIInstance *inst = NULL; - const char *sys = NULL; + const char *id = NULL; - if (cu_get_str_path(reference, "System", &sys) != CMPI_RC_OK) { + if (cu_get_str_path(reference, "System", &id) != CMPI_RC_OK) { cu_statusf(broker, &s, CMPI_RC_ERR_NOT_FOUND, - "No such instance (System)"); + "No such instance (Name)"); goto out; } - s = get_console_sap_by_name(broker, reference, sys, &inst); + s = get_console_sap_by_name(broker, reference, id, &inst); if (s.rc != CMPI_RC_OK) goto out;
participants (2)
-
Dan Smith -
Kaitlin Rupert