On 11/19/2010 07:55 AM, Eduardo Otubo wrote:

I am moving all the UUID handling functions to phyp_uuid.[ch] files in order not to bloat the main files phyp_driver.[ch] too much. Doing this for two reasons:

1) Network management in pHyp does not have a UUID. 2) Need to create another set of functions to manage it.

I also modified some functions to support two types of execution: DOMAIN and NET, so I can re-use the base common functions. --- po/POTFILES.in | 1 + src/Makefile.am | 3 +- src/phyp/phyp_driver.c | 464 +--------------------------------- src/phyp/phyp_driver.h | 41 +++ src/phyp/phyp_uuid.c | 657 ++++++++++++++++++++++++++++++++++++++++++++++++ src/phyp/phyp_uuid.h | 36 +++ 6 files changed, 742 insertions(+), 460 deletions(-) create mode 100644 src/phyp/phyp_uuid.c create mode 100644 src/phyp/phyp_uuid.h

[I've rearranged my review a bit; .h before .c]

diff --git a/src/phyp/phyp_uuid.h b/src/phyp/phyp_uuid.h new file mode 100644 index 0000000..ddf28f4 --- /dev/null +++ b/src/phyp/phyp_uuid.h @@ -0,0 +1,36 @@ + +/* + * Copyright (C) 2010 Red Hat, Inc. + * Copyright IBM Corp. 2010 + * + * phyp_uuid.c: set of functions to handle lpar uuid and network uuid + * which does not have uuid itself, it must be artificially + * created. + * ... + +#include <config.h>

While there are other counter-examples currently in libvirt.git, the general rule of thumb tends to be that .c files should include config.h first before any headers, and therefore .h files should not include it (because it will already have been included by the .c file including this .h).

+ +int phypUUIDTable_Init(virConnectPtr conn);

Where is virConnectPtr defined? This header should be self-contained, rather than relying on the .c file to include pre-requisite headers.

+ +void phypUUIDTable_Free(uuid_tablePtr uuid_table);

Where is uuid_tablePtr defined? Did you mean int? Or should phypUUIDTable_Init be returning a uuid_tablePtr instead of an int?

+ +int phypUUIDTable_RemLpar(virConnectPtr conn, int id); + +int phypUUIDTable_AddLpar(virConnectPtr conn, unsigned char *uuid, int id);

diff --git a/src/phyp/phyp_driver.h b/src/phyp/phyp_driver.h index bc8e003..603d048 100644 --- a/src/phyp/phyp_driver.h +++ b/src/phyp/phyp_driver.h @@ -34,6 +34,7 @@ # define LPAR_EXEC_ERR -1 # define SSH_CONN_ERR -2 /* error while trying to connect to remote host */ # define SSH_CMD_ERR -3 /* error while trying to execute the remote cmd */ +# define NETNAME_SIZE 24

Is this adequate? What's your rationale for this size?

typedef struct _ConnectionData ConnectionData; typedef ConnectionData *ConnectionDataPtr; @@ -42,6 +43,28 @@ struct _ConnectionData { int sock; };

+ +/* This is the network struct that relates + * the MAC with UUID generated by the API + * */ +typedef struct _net net_t; +typedef net_t *netPtr; +struct _net { + unsigned char uuid[VIR_UUID_BUFLEN]; + long long mac;

Typically a mac is 6 bytes, not 8. Is sign extension going to be a problem?

+ char name[NETNAME_SIZE]; +}; + +/* Struct that holds how many networks we're + * handling and a pointer to an array of net structs + * */ +typedef struct _uuid_nettable uuid_nettable_t; +typedef uuid_nettable_t *uuid_nettablePtr; +struct _uuid_nettable { + int nnets;

+ netPtr *nets; +}; + /* This is the lpar (domain) struct that relates * the ID with UUID generated by the API * */ @@ -68,6 +91,7 @@ typedef struct _phyp_driver phyp_driver_t; typedef phyp_driver_t *phyp_driverPtr; struct _phyp_driver { uuid_tablePtr uuid_table; + uuid_nettablePtr uuid_nettable; virCapsPtr caps; int vios_id;

@@ -81,4 +105,21 @@ struct _phyp_driver {

int phypRegister(void);

+ +/* + * Functions used in the phyp_uuid.c and must be visible outside

s/int/size_t/ phyp_driver.c

+ * */ +int phypNumDomainsGeneric(virConnectPtr conn, unsigned int type); + +int phypListDomainsGeneric(virConnectPtr conn, int *ids, int nids, + unsigned int type); + +int waitsocket(int socket_fd, LIBSSH2_SESSION * session);

Why is this not in the phyp namespace?

+ +int phypNumOfNetworks(virConnectPtr conn); + +int phypListNetworks(virConnectPtr conn, char **const names, int nnames); + +int phypListNetworkMAC(virConnectPtr conn, long long *macs, int nnets); + #endif /* PHYP_DRIVER_H */

diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 4c723a2..6f3f49d 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -59,8 +59,10 @@ #include "storage_conf.h" #include "nodeinfo.h" #include "files.h" +#include "network_conf.h"

#include "phyp_driver.h" +#include "phyp_uuid.h"

#define VIR_FROM_THIS VIR_FROM_PHYP

@@ -75,7 +77,7 @@ static unsigned const int HMC = 0; static unsigned const int IVM = 127;

-static int +int waitsocket(int socket_fd, LIBSSH2_SESSION * session)

See earlier question about namespace. You should probably rename it in a separate patch prior to this one, so that the bulk of this patch will be just code motion with no renaming. In fact, when doing a refactor like this, splitting it into multiple steps makes it easier to review: + function renames and signature changes (for example, phypUUIDTable_WriteFile gaining an int type parameter), even though the new parameters have a constant value of DOMAIN + code motion into new files + add new code, by adding support for NET value alongside existing DOMAIN value of parameters added in first patch That way, the two real changes have a smaller line count, and the large line-count code motion change provably has 0 semantic change because it has no subtle differences introduced between the old and new location. That said, I'll go ahead and glance through the rest of this, but my review is very spotty because I'd like to see it broken into three commits before I do a thorough review (in fact, it looks like your static helper functions add NET support in this patch, but that you didn't ever pass NET to those functions until a later patch; my suggestion is therefore to refactor it to have the NET code in the static functions added at the same time or just before the code that uses it, but not during the code motion).

static int -phypUUIDTable_WriteFile(virConnectPtr conn) -{ - phyp_driverPtr phyp_driver = conn->privateData; - uuid_tablePtr uuid_table = phyp_driver->uuid_table; - unsigned int i = 0; - int fd = -1; - char local_file[] = "./uuid_table"; - - if ((fd = creat(local_file, 0755)) == -1)

Pre-existing code question: Why is the ./uuid_table file created with executable permissions? And why world-readable - is any process with a different euid ever expected to read it?

diff --git a/src/phyp/phyp_uuid.c b/src/phyp/phyp_uuid.c new file mode 100644 index 0000000..a97dd44 --- /dev/null +++ b/src/phyp/phyp_uuid.c @@ -0,0 +1,657 @@ + +/* + * Copyright (C) 2010 Red Hat, Inc. + * Copyright IBM Corp. 2010 + * ... +#include <config.h> + +#include <stdlib.h> +#include <stdio.h> +#include <libssh2.h>

Does configure.ac guarantee that this header exists if phyp is enabled, or are you lacking include guards? [answering myself] yes, configure.ac guarantees libssh2 for phyp, so no problem here.

+#include <domain_event.h>

This is an internal file rather than a system header; it should be "domain_event.h".

+static unsigned const int NET = 0; +static unsigned const int DOMAIN = 1;

Generally, I prefer enums over static ints; also, I would have put DOMAIN as 0 and NET as 1 since NET is the new code and since NET sorts after DOMAIN.

+static int +phypUUIDTable_ReadFile(virConnectPtr conn, int type) +{ + phyp_driverPtr phyp_driver = conn->privateData; + unsigned int i = 0; + int fd = -1; + int rc = 0; + + if (type == DOMAIN) { + char local_file[] = "./uuid_table";

This repetition of file names is begging for a #DEFINE.

+ +static int +phypUUIDTable_Push(virConnectPtr conn, int type) +{ + ConnectionData *connection_data = conn->networkPrivateData; + LIBSSH2_SESSION *session = connection_data->session; + LIBSSH2_CHANNEL *channel = NULL; + virBuffer username = VIR_BUFFER_INITIALIZER; + struct stat local_fileinfo; + char buffer[1024]; + int rc = 0; + FILE *fd; + size_t nread, sent; + char *ptr; + char *remote_file = NULL; + char *local_file = NULL; + + if (conn->uri->user != NULL) { + virBufferVSprintf(&username, "%s", conn->uri->user);

Seems like a case for strdup() rather than the overhead of a virBuffer, if all the buffer will ever hold is a copy of a single other string.

+ + if (virBufferError(&username)) { + virBufferFreeAndReset(&username); + virReportOOMError(); + goto err; + } + } + + if (type == DOMAIN) { + if (virAsprintf(&local_file, "./uuid_table") < 0) {

Likewise, virAsprintf seems like overkill when strdup() would do the job.

+ + if (stat(local_file, &local_fileinfo) == -1) { + VIR_WARN0("Unable to stat local file."); + goto err; + } + + if (!(fd = fopen(local_file, "rb"))) {

No need to stat() a file if you are just going to follow it with fopen(); just open it, and fopen() will fail with the same reason as stat() would have failed if it can't be done.

+ VIR_WARN0("Unable to open local file."); + goto err; + } + + do { + channel = + libssh2_scp_send(session, remote_file, + 0x1FF & local_fileinfo.st_mode,

File modes should usually be treated as octal, not hex constants (0777 rather than 0x1ff).

+ +int +phypUUIDTable_AddLpar(virConnectPtr conn, unsigned char *uuid, int id) +{ + phyp_driverPtr phyp_driver = conn->privateData; + uuid_tablePtr uuid_table = phyp_driver->uuid_table; + + uuid_table->nlpars++; + unsigned int i = uuid_table->nlpars; + i--;

Why not just: unsigned int i = uuid_table->nlpars - 1;

+ + uuid_table->lpars[i]->id = id; + memmove(uuid_table->lpars[i]->uuid, uuid, VIR_UUID_BUFLEN);

These don't overlap, you can use memcpy() for speed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

This is the implementation of the previous patch now using virInterface* API. Ended up this patch got much more simpler, smaller and easier to review. Here is some details: * MAC size and interface name are fixed due to specifications on HMC, both are created automatically and CAN'T be specified from user. They have the following format: * MAC: 122980003002 * Interface name: U9124.720.067BE8B-V3-C0 * I did replaced all the |grep|sed following the comments Eric Blake did on the last patch. * According to my last email, It's not possible to create a network interface without assigning it to a specific lpar. Then, I am using this very minimalistic XML file for testing: <interface type='ethernet' name='LPAR01'> </interface> In this file I am using "name" as the lpar name which I am going to assign the new network interface. I couldn't find a better way to refer to it. Comments are welcome. * Regarding the fact I am sleeping one second waiting for the HMC to complete creation of the interface, I don't have means to check if the whole process is done. All I do is execute a command, wait until is complete (which is not enough in this case) check the return and the exit status. The process of actually creating a networking interface seems to take a little longer than just the return of the ssh control. --- src/phyp/phyp_driver.c | 572 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 553 insertions(+), 19 deletions(-) diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 4c723a2..407f644 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -59,6 +59,7 @@ #include "storage_conf.h" #include "nodeinfo.h" #include "files.h" +#include "interface_conf.h" #include "phyp_driver.h" @@ -74,6 +75,8 @@ static unsigned const int HMC = 0; static unsigned const int IVM = 127; +static unsigned const int PHYP_IFACENAME_SIZE = 24; +static unsigned const int PHYP_MAC_SIZE= 12; static int waitsocket(int socket_fd, LIBSSH2_SESSION * session) @@ -3268,6 +3271,542 @@ phypGetStoragePoolXMLDesc(virStoragePoolPtr pool, unsigned int flags) } static int +phypInterfaceDestroy(virInterfacePtr iface, + unsigned int flags ATTRIBUTE_UNUSED) +{ + ConnectionData *connection_data = iface->conn->networkPrivateData; + phyp_driverPtr phyp_driver = iface->conn->privateData; + LIBSSH2_SESSION *session = connection_data->session; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *managed_system = phyp_driver->managed_system; + int system_type = phyp_driver->system_type; + int exit_status = 0; + int slot_num = 0; + int lpar_id = 0; + char *char_ptr; + char *cmd = NULL; + char *ret = NULL; + + /* Getting the remote slot number */ + + char_ptr = NULL; + char_ptr = strchr(iface->mac, '\n'); + + if (char_ptr) + *char_ptr = '\0'; + + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype eth --level lpar " + " -F mac_addr,slot_num|" + " sed -n '/%s/ s/^.*,//p'", iface->mac); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, iface->conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &slot_num) == -1) + goto err; + + /* Getting the remote slot number */ + + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype eth --level lpar " + " -F mac_addr,lpar_id|" + " sed -n '/%s/ s/^.*,//p'", iface->mac); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, iface->conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &lpar_id) == -1) + goto err; + + /* excluding interface */ + + virBufferAddLit(&buf, "chhwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype eth" + " --id %d -o r -s %d", lpar_id, slot_num); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, iface->conn); + + if (exit_status < 0 || ret != NULL) + goto err; + + VIR_FREE(cmd); + VIR_FREE(ret); + return 0; + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return -1; +} + +static virInterfacePtr +phypInterfaceDefineXML(virConnectPtr conn, const char *xml, + unsigned int flags ATTRIBUTE_UNUSED) +{ + ConnectionData *connection_data = conn->networkPrivateData; + phyp_driverPtr phyp_driver = conn->privateData; + LIBSSH2_SESSION *session = connection_data->session; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *managed_system = phyp_driver->managed_system; + int system_type = phyp_driver->system_type; + int exit_status = 0; + char *char_ptr; + char *cmd = NULL; + int slot = 0; + char *ret = NULL; + char *name = NULL; + char *mac = NULL; + virInterfaceDefPtr def; + + if (VIR_ALLOC_N(name, PHYP_IFACENAME_SIZE) < 0) { + virReportOOMError(); + goto err; + } + + if (VIR_ALLOC_N(mac, PHYP_MAC_SIZE) < 0) { + virReportOOMError(); + goto err; + } + + if (!(def = virInterfaceDefParseString(xml))) + goto err; + + /* Now need to get the next free slot number */ + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype slot --level slot" + " -Fslot_num --filter lpar_names=%s" + " |sort|tail -n 1", def->name); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + goto err; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1) + goto err; + + /* The next free slot itself: */ + slot++; + + /* Now addig the new network interface */ + virBufferAddLit(&buf, "chhwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype eth" + " -p %s -o a -s %d -a port_vlan_id=1," + "ieee_virtual_eth=0", def->name, slot); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + goto err; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret != NULL) + goto err; + + /* Need to sleep a little while to wait for the HMC to + * complete the execution of the command. + * */ + sleep(1); + + /* Getting the new interface name */ + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype slot --level slot" + " |sed '/lpar_name=%s/!d; /slot_num=%d/!d; " + "s/^.*drc_name=//'", def->name, slot); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + goto err; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + char_ptr = NULL; + char_ptr = strchr(ret, '\n'); + + if (char_ptr) + *char_ptr = '\0'; + + if (memcpy(name, ret, PHYP_IFACENAME_SIZE) == NULL) + goto err; + + /* Getting the new interface mac addr */ + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + "-r virtualio --rsubtype eth --level lpar " + " |sed '/lpar_name=%s/!d; /slot_num=%d/!d; " + "s/^.*mac_addr=//'", def->name, slot); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + goto err; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (memcpy(mac, ret, PHYP_IFACENAME_SIZE) == NULL) + goto err; + + VIR_FREE(cmd); + VIR_FREE(ret); + return virGetInterface(conn, name, mac); + + err: + VIR_FREE(name); + VIR_FREE(cmd); + VIR_FREE(ret); + return NULL; +} + +static virInterfacePtr +phypInterfaceLookupByName(virConnectPtr conn, const char *name) +{ + ConnectionData *connection_data = conn->networkPrivateData; + phyp_driverPtr phyp_driver = conn->privateData; + LIBSSH2_SESSION *session = connection_data->session; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *managed_system = phyp_driver->managed_system; + int system_type = phyp_driver->system_type; + int exit_status = 0; + char *char_ptr; + char *cmd = NULL; + char *ret = NULL; + int slot = 0; + int lpar_id = 0; + + /*Getting the slot number for the interface */ + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype slot --level slot " + " -F drc_name,slot_num |" + " sed -n '/%s/ s/^.*,//p'", name); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return NULL; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1) + goto err; + + if (char_ptr) + *char_ptr = '\0'; + + /*Getting the lpar_id for the interface */ + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype slot --level slot " + " -F drc_name,lpar_id |" + " sed -n '/%s/ s/^.*,//p'", name); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return NULL; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &lpar_id) == -1) + goto err; + + if (char_ptr) + *char_ptr = '\0'; + + /*Getting the interface mac */ + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype eth --level lpar " + " -F lpar_id,slot_num,mac_addr|" + " sed -n '/%d,%d/ s/^.*,//p'", lpar_id, slot); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return NULL; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + VIR_FREE(cmd); + return virGetInterface(conn, name, ret); + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return NULL; + +} + +static int +phypInterfaceIsActive(virInterfacePtr iface) +{ + ConnectionData *connection_data = iface->conn->networkPrivateData; + phyp_driverPtr phyp_driver = iface->conn->privateData; + LIBSSH2_SESSION *session = connection_data->session; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *managed_system = phyp_driver->managed_system; + int system_type = phyp_driver->system_type; + int exit_status = 0; + int state = 0; + char *char_ptr; + char *cmd = NULL; + char *ret = NULL; + + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype eth --level lpar " + " -F mac_addr,state |" + " sed -n '/%s/ s/^.*,//p'", iface->mac); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, iface->conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &state) == -1) + goto err; + + if (char_ptr) + *char_ptr = '\0'; + + VIR_FREE(cmd); + return state; + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return -1; + +} + +static int +phypListInterfaces(virConnectPtr conn, char **const names, int nnames) +{ + ConnectionData *connection_data = conn->networkPrivateData; + phyp_driverPtr phyp_driver = conn->privateData; + LIBSSH2_SESSION *session = connection_data->session; + int system_type = phyp_driver->system_type; + char *managed_system = phyp_driver->managed_system; + int vios_id = phyp_driver->vios_id; + int exit_status = 0; + int got = 0; + int i; + char *cmd = NULL; + char *ret = NULL; + char *networks = NULL; + char *char_ptr2 = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + virBufferAddLit(&buf, "lshwres"); + if (system_type == HMC) + virBufferVSprintf(&buf, " -m %s", managed_system); + virBufferVSprintf(&buf, " -r virtualio --rsubtype slot --level slot|" + " sed '/eth/!d; /lpar_id=%d/d; s/^.*drc_name=//g'", + vios_id); + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + /* I need to parse the textual return in order to get the network interfaces */ + if (exit_status < 0 || ret == NULL) + goto err; + else { + networks = ret; + + while (got < nnames) { + char_ptr2 = strchr(networks, '\n'); + + if (char_ptr2) { + *char_ptr2 = '\0'; + if ((names[got++] = strdup(networks)) == NULL) { + virReportOOMError(); + goto err; + } + char_ptr2++; + networks = char_ptr2; + } else + break; + } + } + + VIR_FREE(cmd); + VIR_FREE(ret); + return got; + + err: + for (i = 0; i < got; i++) + VIR_FREE(names[i]); + VIR_FREE(cmd); + VIR_FREE(ret); + return -1; +} + +static int +phypNumOfInterfaces(virConnectPtr conn) +{ + ConnectionData *connection_data = conn->networkPrivateData; + phyp_driverPtr phyp_driver = conn->privateData; + LIBSSH2_SESSION *session = connection_data->session; + char *managed_system = phyp_driver->managed_system; + int system_type = phyp_driver->system_type; + int vios_id = phyp_driver->vios_id; + int exit_status = 0; + int nnets = 0; + char *char_ptr; + char *cmd = NULL; + char *ret = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + "-r virtualio --rsubtype eth --level lpar|" + "grep -v lpar_id=%d|grep -c lpar_name", vios_id); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &nnets) == -1) + goto err; + + if (char_ptr) + *char_ptr = '\0'; + + VIR_FREE(cmd); + return nnets; + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return -1; + +} + +static int phypGetLparState(virConnectPtr conn, unsigned int lpar_id) { ConnectionData *connection_data = conn->networkPrivateData; @@ -4087,27 +4626,22 @@ static virStorageDriver phypStorageDriver = { .poolIsPersistent = NULL }; -static virNetworkDriver phypNetworkDriver = { +static virInterfaceDriver phypInterfaceDriver = { .name = "PHYP", .open = phypVIOSDriverOpen, .close = phypVIOSDriverClose, - .numOfNetworks = NULL, - .listNetworks = NULL, - .numOfDefinedNetworks = NULL, - .listDefinedNetworks = NULL, - .networkLookupByUUID = NULL, - .networkLookupByName = NULL, - .networkCreateXML = NULL, - .networkDefineXML = NULL, - .networkUndefine = NULL, - .networkCreate = NULL, - .networkDestroy = NULL, - .networkDumpXML = NULL, - .networkGetBridgeName = NULL, - .networkGetAutostart = NULL, - .networkSetAutostart = NULL, - .networkIsActive = NULL, - .networkIsPersistent = NULL + .numOfInterfaces = phypNumOfInterfaces, + .listInterfaces = phypListInterfaces, + .numOfDefinedInterfaces = NULL, + .listDefinedInterfaces = NULL, + .interfaceLookupByName = phypInterfaceLookupByName, + .interfaceLookupByMACString = NULL, + .interfaceGetXMLDesc = NULL, + .interfaceDefineXML = phypInterfaceDefineXML, + .interfaceUndefine = NULL, + .interfaceCreate = NULL, + .interfaceDestroy = phypInterfaceDestroy, + .interfaceIsActive = phypInterfaceIsActive }; int @@ -4117,7 +4651,7 @@ phypRegister(void) return -1; if (virRegisterStorageDriver(&phypStorageDriver) < 0) return -1; - if (virRegisterNetworkDriver(&phypNetworkDriver) < 0) + if (virRegisterInterfaceDriver(&phypInterfaceDriver) < 0) return -1; return 0; -- 1.7.1

On 11/22/2010 06:03 PM, Eduardo Otubo wrote: Apologies for my review backlog (if you haven't guessed, I sometimes table big patches for later, then forget to come back rapidly).

This is the implementation of the previous patch now using virInterface* API. Ended up this patch got much more simpler, smaller and easier to review. Here is some details:

* MAC size and interface name are fixed due to specifications on HMC, both are created automatically and CAN'T be specified from user. They have the following format:

* MAC: 122980003002

HMC represents MAC as a decimal number? How hideous, but not your fault; and this comment in the commit message will be useful for anyone auditing the code in the future.

+ /* Getting the remote slot number */ + + char_ptr = NULL; + char_ptr = strchr(iface->mac, '\n');

Redundant assignments (delete the first line).

+ if (VIR_ALLOC_N(name, PHYP_IFACENAME_SIZE) < 0) { + virReportOOMError(); + goto err; + } + + if (VIR_ALLOC_N(mac, PHYP_MAC_SIZE) < 0) { + virReportOOMError(); + goto err; + }

Since these arrays are so small, it's faster to just stack-allocate them: char name[PHYP_IFACENAME_SIZE];

+ /* The next free slot itself: */ + slot++; + + /* Now addig the new network interface */

s/addig/adding/

+ if (exit_status < 0 || ret == NULL) + goto err; + + char_ptr = NULL; + char_ptr = strchr(ret, '\n');

Another redundant assignment.

+ if (memcpy(mac, ret, PHYP_IFACENAME_SIZE) == NULL) + goto err; + + VIR_FREE(cmd); + VIR_FREE(ret); + return virGetInterface(conn, name, mac); + + err: + VIR_FREE(name); + VIR_FREE(cmd); + VIR_FREE(ret); + return NULL;

This leaks name if you end up calling virGetInterface. And both paths leak mac. But if you change name and mac to be stack-allocated, then you don't have to worry about freeing them.

+ ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + VIR_FREE(cmd); + return virGetInterface(conn, name, ret); + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return NULL;

This leaks ret if virGetInterface is called.

+ ret = phypExec(session, cmd, &exit_status, iface->conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &state) == -1) + goto err; + + if (char_ptr) + *char_ptr = '\0'; + + VIR_FREE(cmd); + return state;

Another leak of ret.

+static int +phypListInterfaces(virConnectPtr conn, char **const names, int nnames)

s/int/size_t/

+ ret = phypExec(session, cmd, &exit_status, conn); + + /* I need to parse the textual return in order to get the network interfaces */ + if (exit_status < 0 || ret == NULL) + goto err; + else {

Rather than indenting the rest of the function inside an else{} block, you can leave the code at the top level indentation since the if() block was an unconditional goto.

+ ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &nnets) == -1) + goto err; + + if (char_ptr) + *char_ptr = '\0'; + + VIR_FREE(cmd); + return nnets;

Another leak of ret.

int @@ -4117,7 +4651,7 @@ phypRegister(void) return -1; if (virRegisterStorageDriver(&phypStorageDriver) < 0) return -1; - if (virRegisterNetworkDriver(&phypNetworkDriver) < 0) + if (virRegisterInterfaceDriver(&phypInterfaceDriver) < 0)

Are you intending to replace NetworkDriver with InterfaceDriver, or should you be supporting both drivers simultaneously (although this may be more an indication of how unfamiliar I am with the difference between what the two drivers are supposed to provide). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

This is the implementation of the previous patch now using virInterface* API. Ended up this patch got much more simpler, smaller and easier to review. Here is some details: * MAC size and interface name are fixed due to specifications on HMC, both are created automatically and CAN'T be specified from user. They have the following format: * MAC: 122980003002 * Interface name: U9124.720.067BE8B-V3-C0 * I did replaced all the |grep|sed following the comments Eric Blake did on the last patch. * According to my last email, It's not possible to create a network interface without assigning it to a specific lpar. Then, I am using this very minimalistic XML file for testing: <interface type='ethernet' name='LPAR01'> </interface> In this file I am using "name" as the lpar name which I am going to assign the new network interface. I couldn't find a better way to refer to it. Comments are welcome. * Regarding the fact I am sleeping one second waiting for the HMC to complete creation of the interface, I don't have means to check if the whole process is done. All I do is execute a command, wait until is complete (which is not enough in this case) check the return and the exit status. The process of actually creating a networking interface seems to take a little longer than just the return of the ssh control. --- src/phyp/phyp_driver.c | 565 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 546 insertions(+), 19 deletions(-) diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 3508891..40760a5 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -59,6 +59,7 @@ #include "storage_conf.h" #include "nodeinfo.h" #include "files.h" +#include "interface_conf.h" #include "phyp_driver.h" @@ -74,6 +75,8 @@ static unsigned const int HMC = 0; static unsigned const int IVM = 127; +static unsigned const int PHYP_IFACENAME_SIZE = 24; +static unsigned const int PHYP_MAC_SIZE= 12; static int waitsocket(int socket_fd, LIBSSH2_SESSION * session) @@ -3273,6 +3276,535 @@ phypGetStoragePoolXMLDesc(virStoragePoolPtr pool, unsigned int flags) } static int +phypInterfaceDestroy(virInterfacePtr iface, + unsigned int flags ATTRIBUTE_UNUSED) +{ + ConnectionData *connection_data = iface->conn->networkPrivateData; + phyp_driverPtr phyp_driver = iface->conn->privateData; + LIBSSH2_SESSION *session = connection_data->session; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *managed_system = phyp_driver->managed_system; + int system_type = phyp_driver->system_type; + int exit_status = 0; + int slot_num = 0; + int lpar_id = 0; + char *char_ptr; + char *cmd = NULL; + char *ret = NULL; + + /* Getting the remote slot number */ + + char_ptr = strchr(iface->mac, '\n'); + + if (char_ptr) + *char_ptr = '\0'; + + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype eth --level lpar " + " -F mac_addr,slot_num|" + " sed -n '/%s/ s/^.*,//p'", iface->mac); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, iface->conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &slot_num) == -1) + goto err; + + /* Getting the remote slot number */ + + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype eth --level lpar " + " -F mac_addr,lpar_id|" + " sed -n '/%s/ s/^.*,//p'", iface->mac); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, iface->conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &lpar_id) == -1) + goto err; + + /* excluding interface */ + + virBufferAddLit(&buf, "chhwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype eth" + " --id %d -o r -s %d", lpar_id, slot_num); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, iface->conn); + + if (exit_status < 0 || ret != NULL) + goto err; + + VIR_FREE(cmd); + VIR_FREE(ret); + return 0; + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return -1; +} + +static virInterfacePtr +phypInterfaceDefineXML(virConnectPtr conn, const char *xml, + unsigned int flags ATTRIBUTE_UNUSED) +{ + ConnectionData *connection_data = conn->networkPrivateData; + phyp_driverPtr phyp_driver = conn->privateData; + LIBSSH2_SESSION *session = connection_data->session; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *managed_system = phyp_driver->managed_system; + int system_type = phyp_driver->system_type; + int exit_status = 0; + char *char_ptr; + char *cmd = NULL; + int slot = 0; + char *ret = NULL; + char name[PHYP_IFACENAME_SIZE]; + char mac[PHYP_MAC_SIZE]; + virInterfaceDefPtr def; + + if (!(def = virInterfaceDefParseString(xml))) + goto err; + + /* Now need to get the next free slot number */ + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype slot --level slot" + " -Fslot_num --filter lpar_names=%s" + " |sort|tail -n 1", def->name); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + goto err; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1) + goto err; + + /* The next free slot itself: */ + slot++; + + /* Now adding the new network interface */ + virBufferAddLit(&buf, "chhwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype eth" + " -p %s -o a -s %d -a port_vlan_id=1," + "ieee_virtual_eth=0", def->name, slot); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + goto err; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret != NULL) + goto err; + + /* Need to sleep a little while to wait for the HMC to + * complete the execution of the command. + * */ + sleep(1); + + /* Getting the new interface name */ + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype slot --level slot" + " |sed '/lpar_name=%s/!d; /slot_num=%d/!d; " + "s/^.*drc_name=//'", def->name, slot); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + goto err; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + char_ptr = strchr(ret, '\n'); + + if (char_ptr) + *char_ptr = '\0'; + + if (memcpy(name, ret, PHYP_IFACENAME_SIZE-1) == NULL) + goto err; + + /* Getting the new interface mac addr */ + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + "-r virtualio --rsubtype eth --level lpar " + " |sed '/lpar_name=%s/!d; /slot_num=%d/!d; " + "s/^.*mac_addr=//'", def->name, slot); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + goto err; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (memcpy(mac, ret, PHYP_MAC_SIZE-1) == NULL) + goto err; + + VIR_FREE(cmd); + VIR_FREE(ret); + return virGetInterface(conn, name, mac); + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return NULL; +} + +static virInterfacePtr +phypInterfaceLookupByName(virConnectPtr conn, const char *name) +{ + ConnectionData *connection_data = conn->networkPrivateData; + phyp_driverPtr phyp_driver = conn->privateData; + LIBSSH2_SESSION *session = connection_data->session; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *managed_system = phyp_driver->managed_system; + int system_type = phyp_driver->system_type; + int exit_status = 0; + char *char_ptr; + char *cmd = NULL; + char *ret = NULL; + int slot = 0; + int lpar_id = 0; + char mac[PHYP_MAC_SIZE]; + + /*Getting the slot number for the interface */ + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype slot --level slot " + " -F drc_name,slot_num |" + " sed -n '/%s/ s/^.*,//p'", name); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return NULL; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1) + goto err; + + if (char_ptr) + *char_ptr = '\0'; + + /*Getting the lpar_id for the interface */ + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype slot --level slot " + " -F drc_name,lpar_id |" + " sed -n '/%s/ s/^.*,//p'", name); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return NULL; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &lpar_id) == -1) + goto err; + + if (char_ptr) + *char_ptr = '\0'; + + /*Getting the interface mac */ + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype eth --level lpar " + " -F lpar_id,slot_num,mac_addr|" + " sed -n '/%d,%d/ s/^.*,//p'", lpar_id, slot); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return NULL; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (memcpy(mac, ret, PHYP_MAC_SIZE-1) == NULL) + goto err; + + VIR_FREE(cmd); + VIR_FREE(ret); + return virGetInterface(conn, name, ret); + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return NULL; + +} + +static int +phypInterfaceIsActive(virInterfacePtr iface) +{ + ConnectionData *connection_data = iface->conn->networkPrivateData; + phyp_driverPtr phyp_driver = iface->conn->privateData; + LIBSSH2_SESSION *session = connection_data->session; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *managed_system = phyp_driver->managed_system; + int system_type = phyp_driver->system_type; + int exit_status = 0; + int state = 0; + char *char_ptr; + char *cmd = NULL; + char *ret = NULL; + + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype eth --level lpar " + " -F mac_addr,state |" + " sed -n '/%s/ s/^.*,//p'", iface->mac); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, iface->conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &state) == -1) + goto err; + + if (char_ptr) + *char_ptr = '\0'; + + VIR_FREE(cmd); + VIR_FREE(ret); + return state; + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return -1; + +} + +static size_t +phypListInterfaces(virConnectPtr conn, char **const names, int nnames) +{ + ConnectionData *connection_data = conn->networkPrivateData; + phyp_driverPtr phyp_driver = conn->privateData; + LIBSSH2_SESSION *session = connection_data->session; + int system_type = phyp_driver->system_type; + char *managed_system = phyp_driver->managed_system; + int vios_id = phyp_driver->vios_id; + int exit_status = 0; + int got = 0; + int i; + char *cmd = NULL; + char *ret = NULL; + char *networks = NULL; + char *char_ptr2 = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + virBufferAddLit(&buf, "lshwres"); + if (system_type == HMC) + virBufferVSprintf(&buf, " -m %s", managed_system); + virBufferVSprintf(&buf, " -r virtualio --rsubtype slot --level slot|" + " sed '/eth/!d; /lpar_id=%d/d; s/^.*drc_name=//g'", + vios_id); + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + /* I need to parse the textual return in order to get the network interfaces */ + if (exit_status < 0 || ret == NULL) + goto err; + + networks = ret; + + while (got < nnames) { + char_ptr2 = strchr(networks, '\n'); + + if (char_ptr2) { + *char_ptr2 = '\0'; + if ((names[got++] = strdup(networks)) == NULL) { + virReportOOMError(); + goto err; + } + char_ptr2++; + networks = char_ptr2; + } else + break; + } + + VIR_FREE(cmd); + VIR_FREE(ret); + return got; + + err: + for (i = 0; i < got; i++) + VIR_FREE(names[i]); + VIR_FREE(cmd); + VIR_FREE(ret); + return -1; +} + +static int +phypNumOfInterfaces(virConnectPtr conn) +{ + ConnectionData *connection_data = conn->networkPrivateData; + phyp_driverPtr phyp_driver = conn->privateData; + LIBSSH2_SESSION *session = connection_data->session; + char *managed_system = phyp_driver->managed_system; + int system_type = phyp_driver->system_type; + int vios_id = phyp_driver->vios_id; + int exit_status = 0; + int nnets = 0; + char *char_ptr; + char *cmd = NULL; + char *ret = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + "-r virtualio --rsubtype eth --level lpar|" + "grep -v lpar_id=%d|grep -c lpar_name", vios_id); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &nnets) == -1) + goto err; + + if (char_ptr) + *char_ptr = '\0'; + + VIR_FREE(cmd); + VIR_FREE(ret); + return nnets; + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return -1; + +} + +static int phypGetLparState(virConnectPtr conn, unsigned int lpar_id) { ConnectionData *connection_data = conn->networkPrivateData; @@ -4093,27 +4625,22 @@ static virStorageDriver phypStorageDriver = { .poolIsPersistent = NULL }; -static virNetworkDriver phypNetworkDriver = { +static virInterfaceDriver phypInterfaceDriver = { .name = "PHYP", .open = phypVIOSDriverOpen, .close = phypVIOSDriverClose, - .numOfNetworks = NULL, - .listNetworks = NULL, - .numOfDefinedNetworks = NULL, - .listDefinedNetworks = NULL, - .networkLookupByUUID = NULL, - .networkLookupByName = NULL, - .networkCreateXML = NULL, - .networkDefineXML = NULL, - .networkUndefine = NULL, - .networkCreate = NULL, - .networkDestroy = NULL, - .networkDumpXML = NULL, - .networkGetBridgeName = NULL, - .networkGetAutostart = NULL, - .networkSetAutostart = NULL, - .networkIsActive = NULL, - .networkIsPersistent = NULL + .numOfInterfaces = phypNumOfInterfaces, + .listInterfaces = phypListInterfaces, + .numOfDefinedInterfaces = NULL, + .listDefinedInterfaces = NULL, + .interfaceLookupByName = phypInterfaceLookupByName, + .interfaceLookupByMACString = NULL, + .interfaceGetXMLDesc = NULL, + .interfaceDefineXML = phypInterfaceDefineXML, + .interfaceUndefine = NULL, + .interfaceCreate = NULL, + .interfaceDestroy = phypInterfaceDestroy, + .interfaceIsActive = phypInterfaceIsActive }; int @@ -4123,7 +4650,7 @@ phypRegister(void) return -1; if (virRegisterStorageDriver(&phypStorageDriver) < 0) return -1; - if (virRegisterNetworkDriver(&phypNetworkDriver) < 0) + if (virRegisterInterfaceDriver(&phypInterfaceDriver) < 0) return -1; return 0; -- 1.7.1

Any chance to get a review for the next release? Thanks a lot :-) Regards, On 12/29/2010 04:04 PM, Eduardo Otubo wrote:

This is the implementation of the previous patch now using virInterface* API. Ended up this patch got much more simpler, smaller and easier to review. Here is some details:

* MAC size and interface name are fixed due to specifications on HMC, both are created automatically and CAN'T be specified from user. They have the following format:

* MAC: 122980003002 * Interface name: U9124.720.067BE8B-V3-C0

* I did replaced all the |grep|sed following the comments Eric Blake did on the last patch.

* According to my last email, It's not possible to create a network interface without assigning it to a specific lpar. Then, I am using this very minimalistic XML file for testing:

<interface type='ethernet' name='LPAR01'> </interface>

In this file I am using "name" as the lpar name which I am going to assign the new network interface. I couldn't find a better way to refer to it. Comments are welcome.

* Regarding the fact I am sleeping one second waiting for the HMC to complete creation of the interface, I don't have means to check if the whole process is done. All I do is execute a command, wait until is complete (which is not enough in this case) check the return and the exit status. The process of actually creating a networking interface seems to take a little longer than just the return of the ssh control. --- src/phyp/phyp_driver.c | 572 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 551 insertions(+), 21 deletions(-)

diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 3508891..62a38a2 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -59,6 +59,7 @@ #include "storage_conf.h" #include "nodeinfo.h" #include "files.h" +#include "interface_conf.h"

#include "phyp_driver.h"

@@ -74,6 +75,11 @@

static unsigned const int HMC = 0; static unsigned const int IVM = 127; +static unsigned const int PHYP_IFACENAME_SIZE = 24; +static unsigned const int PHYP_MAC_SIZE= 12; + + +virCheckFlags(0,NULL);

static int waitsocket(int socket_fd, LIBSSH2_SESSION * session) @@ -1113,7 +1119,7 @@ openSSHSession(virConnectPtr conn, virConnectAuthPtr auth,

static virDrvOpenStatus phypOpen(virConnectPtr conn, - virConnectAuthPtr auth, int flags ATTRIBUTE_UNUSED) + virConnectAuthPtr auth, int flags) { LIBSSH2_SESSION *session = NULL; ConnectionData *connection_data = NULL; @@ -3273,6 +3279,535 @@ phypGetStoragePoolXMLDesc(virStoragePoolPtr pool, unsigned int flags) }

static int +phypInterfaceDestroy(virInterfacePtr iface, + unsigned int flags) +{ + ConnectionData *connection_data = iface->conn->networkPrivateData; + phyp_driverPtr phyp_driver = iface->conn->privateData; + LIBSSH2_SESSION *session = connection_data->session; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *managed_system = phyp_driver->managed_system; + int system_type = phyp_driver->system_type; + int exit_status = 0; + int slot_num = 0; + int lpar_id = 0; + char *char_ptr; + char *cmd = NULL; + char *ret = NULL; + + /* Getting the remote slot number */ + + char_ptr = strchr(iface->mac, '\n'); + + if (char_ptr) + *char_ptr = '\0'; + + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype eth --level lpar " + " -F mac_addr,slot_num|" + " sed -n '/%s/ s/^.*,//p'", iface->mac); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + goto err; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd,&exit_status, iface->conn); + + if (exit_status< 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret,&char_ptr, 10,&slot_num) == -1) + goto err; + + /* Getting the remote slot number */ + VIR_FREE(cmd); + VIR_FREE(ret); + + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype eth --level lpar " + " -F mac_addr,lpar_id|" + " sed -n '/%s/ s/^.*,//p'", iface->mac); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + goto err; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd,&exit_status, iface->conn); + + if (exit_status< 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret,&char_ptr, 10,&lpar_id) == -1) + goto err; + + /* excluding interface */ + VIR_FREE(cmd); + VIR_FREE(ret); + + virBufferAddLit(&buf, "chhwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype eth" + " --id %d -o r -s %d", lpar_id, slot_num); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd,&exit_status, iface->conn); + + if (exit_status< 0 || ret != NULL) + goto err; + + VIR_FREE(cmd); + VIR_FREE(ret); + return 0; + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return -1; +} + +static virInterfacePtr +phypInterfaceDefineXML(virConnectPtr conn, const char *xml, + unsigned int flags) +{ + ConnectionData *connection_data = conn->networkPrivateData; + phyp_driverPtr phyp_driver = conn->privateData; + LIBSSH2_SESSION *session = connection_data->session; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *managed_system = phyp_driver->managed_system; + int system_type = phyp_driver->system_type; + int exit_status = 0; + char *char_ptr; + char *cmd = NULL; + int slot = 0; + char *ret = NULL; + char name[PHYP_IFACENAME_SIZE]; + char mac[PHYP_MAC_SIZE]; + virInterfaceDefPtr def; + + if (!(def = virInterfaceDefParseString(xml))) + goto err; + + /* Now need to get the next free slot number */ + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype slot --level slot" + " -Fslot_num --filter lpar_names=%s" + " |sort|tail -n 1", def->name); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + goto err; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd,&exit_status, conn); + + if (exit_status< 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret,&char_ptr, 10,&slot) == -1) + goto err; + + /* The next free slot itself: */ + slot++; + + /* Now adding the new network interface */ + virBufferAddLit(&buf, "chhwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype eth" + " -p %s -o a -s %d -a port_vlan_id=1," + "ieee_virtual_eth=0", def->name, slot); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + goto err; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd,&exit_status, conn); + + if (exit_status< 0 || ret != NULL) + goto err; + + /* Need to sleep a little while to wait for the HMC to + * complete the execution of the command. + * */ + sleep(1); + + /* Getting the new interface name */ + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype slot --level slot" + " |sed '/lpar_name=%s/!d; /slot_num=%d/!d; " + "s/^.*drc_name=//'", def->name, slot); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + goto err; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd,&exit_status, conn); + + if (exit_status< 0 || ret == NULL) + goto err; + + char_ptr = strchr(ret, '\n'); + + if (char_ptr) + *char_ptr = '\0'; + + if (memcpy(name, ret, PHYP_IFACENAME_SIZE-1) == NULL) + goto err; + + /* Getting the new interface mac addr */ + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + "-r virtualio --rsubtype eth --level lpar " + " |sed '/lpar_name=%s/!d; /slot_num=%d/!d; " + "s/^.*mac_addr=//'", def->name, slot); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + goto err; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd,&exit_status, conn); + + if (exit_status< 0 || ret == NULL) + goto err; + + if (memcpy(mac, ret, PHYP_MAC_SIZE-1) == NULL) + goto err; + + VIR_FREE(cmd); + VIR_FREE(ret); + return virGetInterface(conn, name, mac); + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return NULL; +} + +static virInterfacePtr +phypInterfaceLookupByName(virConnectPtr conn, const char *name) +{ + ConnectionData *connection_data = conn->networkPrivateData; + phyp_driverPtr phyp_driver = conn->privateData; + LIBSSH2_SESSION *session = connection_data->session; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *managed_system = phyp_driver->managed_system; + int system_type = phyp_driver->system_type; + int exit_status = 0; + char *char_ptr; + char *cmd = NULL; + char *ret = NULL; + int slot = 0; + int lpar_id = 0; + char mac[PHYP_MAC_SIZE]; + + /*Getting the slot number for the interface */ + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype slot --level slot " + " -F drc_name,slot_num |" + " sed -n '/%s/ s/^.*,//p'", name); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return NULL; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd,&exit_status, conn); + + if (exit_status< 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret,&char_ptr, 10,&slot) == -1) + goto err; + + if (char_ptr) + *char_ptr = '\0'; + + /*Getting the lpar_id for the interface */ + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype slot --level slot " + " -F drc_name,lpar_id |" + " sed -n '/%s/ s/^.*,//p'", name); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return NULL; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd,&exit_status, conn); + + if (exit_status< 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret,&char_ptr, 10,&lpar_id) == -1) + goto err; + + if (char_ptr) + *char_ptr = '\0'; + + /*Getting the interface mac */ + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype eth --level lpar " + " -F lpar_id,slot_num,mac_addr|" + " sed -n '/%d,%d/ s/^.*,//p'", lpar_id, slot); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return NULL; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd,&exit_status, conn); + + if (exit_status< 0 || ret == NULL) + goto err; + + if (memcpy(mac, ret, PHYP_MAC_SIZE-1) == NULL) + goto err; + + VIR_FREE(cmd); + return virGetInterface(conn, name, ret); + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return NULL; + +} + +static int +phypInterfaceIsActive(virInterfacePtr iface) +{ + ConnectionData *connection_data = iface->conn->networkPrivateData; + phyp_driverPtr phyp_driver = iface->conn->privateData; + LIBSSH2_SESSION *session = connection_data->session; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *managed_system = phyp_driver->managed_system; + int system_type = phyp_driver->system_type; + int exit_status = 0; + int state = 0; + char *char_ptr; + char *cmd = NULL; + char *ret = NULL; + + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype eth --level lpar " + " -F mac_addr,state |" + " sed -n '/%s/ s/^.*,//p'", iface->mac); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd,&exit_status, iface->conn); + + if (exit_status< 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret,&char_ptr, 10,&state) == -1) + goto err; + + if (char_ptr) + *char_ptr = '\0'; + + VIR_FREE(cmd); + VIR_FREE(ret); + return state; + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return -1; + +} + +static int +phypListInterfaces(virConnectPtr conn, char **const names, int nnames) +{ + ConnectionData *connection_data = conn->networkPrivateData; + phyp_driverPtr phyp_driver = conn->privateData; + LIBSSH2_SESSION *session = connection_data->session; + int system_type = phyp_driver->system_type; + char *managed_system = phyp_driver->managed_system; + int vios_id = phyp_driver->vios_id; + int exit_status = 0; + int got = 0; + int i; + char *cmd = NULL; + char *ret = NULL; + char *networks = NULL; + char *char_ptr2 = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + virBufferAddLit(&buf, "lshwres"); + if (system_type == HMC) + virBufferVSprintf(&buf, " -m %s", managed_system); + virBufferVSprintf(&buf, " -r virtualio --rsubtype slot --level slot|" + " sed '/eth/!d; /lpar_id=%d/d; s/^.*drc_name=//g'", + vios_id); + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + goto err; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd,&exit_status, conn); + + /* I need to parse the textual return in order to get the network interfaces */ + if (exit_status< 0 || ret == NULL) + goto err; + + networks = ret; + + while (got< nnames) { + char_ptr2 = strchr(networks, '\n'); + + if (char_ptr2) { + *char_ptr2 = '\0'; + if ((names[got++] = strdup(networks)) == NULL) { + virReportOOMError(); + goto err; + } + char_ptr2++; + networks = char_ptr2; + } else + break; + } + + VIR_FREE(cmd); + VIR_FREE(ret); + return got; + + err: + for (i = 0; i< got; i++) + VIR_FREE(names[i]); + VIR_FREE(cmd); + VIR_FREE(ret); + return -1; +} + +static int +phypNumOfInterfaces(virConnectPtr conn) +{ + ConnectionData *connection_data = conn->networkPrivateData; + phyp_driverPtr phyp_driver = conn->privateData; + LIBSSH2_SESSION *session = connection_data->session; + char *managed_system = phyp_driver->managed_system; + int system_type = phyp_driver->system_type; + int vios_id = phyp_driver->vios_id; + int exit_status = 0; + int nnets = 0; + char *char_ptr; + char *cmd = NULL; + char *ret = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + "-r virtualio --rsubtype eth --level lpar|" + "grep -v lpar_id=%d|grep -c lpar_name", vios_id); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd,&exit_status, conn); + + if (exit_status< 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret,&char_ptr, 10,&nnets) == -1) + goto err; + + VIR_FREE(cmd); + VIR_FREE(ret); + return nnets; + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return -1; + +} + +static int phypGetLparState(virConnectPtr conn, unsigned int lpar_id) { ConnectionData *connection_data = conn->networkPrivateData; @@ -3932,7 +4467,7 @@ phypDomainSetCPU(virDomainPtr dom, unsigned int nvcpus) static virDrvOpenStatus phypVIOSDriverOpen(virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, - int flags ATTRIBUTE_UNUSED) + int flags) { if (conn->driver->no != VIR_DRV_PHYP) return VIR_DRV_OPEN_DECLINED; @@ -4093,27 +4628,22 @@ static virStorageDriver phypStorageDriver = { .poolIsPersistent = NULL };

-static virNetworkDriver phypNetworkDriver = { +static virInterfaceDriver phypInterfaceDriver = { .name = "PHYP", .open = phypVIOSDriverOpen, .close = phypVIOSDriverClose, - .numOfNetworks = NULL, - .listNetworks = NULL, - .numOfDefinedNetworks = NULL, - .listDefinedNetworks = NULL, - .networkLookupByUUID = NULL, - .networkLookupByName = NULL, - .networkCreateXML = NULL, - .networkDefineXML = NULL, - .networkUndefine = NULL, - .networkCreate = NULL, - .networkDestroy = NULL, - .networkDumpXML = NULL, - .networkGetBridgeName = NULL, - .networkGetAutostart = NULL, - .networkSetAutostart = NULL, - .networkIsActive = NULL, - .networkIsPersistent = NULL + .numOfInterfaces = phypNumOfInterfaces, + .listInterfaces = phypListInterfaces, + .numOfDefinedInterfaces = NULL, + .listDefinedInterfaces = NULL, + .interfaceLookupByName = phypInterfaceLookupByName, + .interfaceLookupByMACString = NULL, + .interfaceGetXMLDesc = NULL, + .interfaceDefineXML = phypInterfaceDefineXML, + .interfaceUndefine = NULL, + .interfaceCreate = NULL, + .interfaceDestroy = phypInterfaceDestroy, + .interfaceIsActive = phypInterfaceIsActive };

int @@ -4123,7 +4653,7 @@ phypRegister(void) return -1; if (virRegisterStorageDriver(&phypStorageDriver)< 0) return -1; - if (virRegisterNetworkDriver(&phypNetworkDriver)< 0) + if (virRegisterInterfaceDriver(&phypInterfaceDriver)< 0) return -1;

return 0;

-- Eduardo Otubo Software Engineer Linux Technology Center IBM Systems & Technology Group Mobile: +55 19 8135 0885 eotubo@linux.vnet.ibm.com

On 01/14/2011 10:07 PM, Eric Blake wrote:

On 12/29/2010 11:04 AM, Eduardo Otubo wrote:

...

This is the implementation of the previous patch now using virInterface* API. Ended up this patch got much more simpler, smaller and easier to review. Here is some details:

Finally getting around to reviewing this again. I'm not intentionally putting you off, it's just that I'm less familiar with phyp and it takes a good chunk of free time to review large patches in areas where I'm less familiar.

No problem, I understand your point on not having a way to test it. In, future work, I'll try to send smaller patches so it gets much easier for you all to review. And sorry for the delay on replying this email, I was on a vacation, so catching up all now.

...

@@ -74,6 +75,11 @@

static unsigned const int HMC = 0; static unsigned const int IVM = 127; +static unsigned const int PHYP_IFACENAME_SIZE = 24; +static unsigned const int PHYP_MAC_SIZE= 12; + + +virCheckFlags(0,NULL);

This is misplaced, and causes a compile error. It should be one of the first statements (not declarations) in any function that takes a flags argument where you do not otherwise use flags, and not something done at the file scope.

...

static int waitsocket(int socket_fd, LIBSSH2_SESSION * session) @@ -1113,7 +1119,7 @@ openSSHSession(virConnectPtr conn, virConnectAuthPtr auth,

static virDrvOpenStatus phypOpen(virConnectPtr conn, - virConnectAuthPtr auth, int flags ATTRIBUTE_UNUSED) + virConnectAuthPtr auth, int flags) { LIBSSH2_SESSION *session = NULL; ConnectionData *connection_data = NULL;

That is, somewhere in this function.

Seeing as how this causes a compile error:

CC libvirt_driver_phyp_la-phyp_driver.lo phyp/phyp_driver.c:82:1: error: expected identifier or '(' before 'do' phyp/phyp_driver.c:82:290: error: expected identifier or '(' before 'while' cc1: warnings being treated as errors phyp/phyp_driver.c: In function 'phypOpen': phyp/phyp_driver.c:1122:38: error: unused parameter 'flags' [-Wunused-parameter] phyp/phyp_driver.c: In function 'phypInterfaceDestroy':

how can I even be sure that you've tested your patch, to spend my time reviewing it?

Works for me. otubo@vader ~ $ uname -a Linux vader 2.6.35-25-generic #44-Ubuntu SMP Fri Jan 21 17:40:48 UTC 2011 i686 GNU/Linux otubo@vader ~/develop/libvirt master $ gcc -v Using built-in specs. Target: i686-linux-gnu Configured with: ../src/configure -v --with-pkgversion='Ubuntu/Linaro 4.4.4-14ubuntu5' --with-bugurl=file:///usr/share/doc/gcc-4.4/README.Bugs --enable-languages=c,c++,fortran,objc,obj-c++ --prefix=/usr --program-suffix=-4.4 --enable-shared --enable-multiarch --enable-linker-build-id --with-system-zlib --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --with-gxx-include-dir=/usr/include/c++/4.4 --libdir=/usr/lib --enable-nls --with-sysroot=/ --enable-clocale=gnu --enable-libstdcxx-debug --enable-objc-gc --enable-targets=all --disable-werror --with-arch-32=i686 --with-tune=generic --enable-checking=release --build=i686-linux-gnu --host=i686-linux-gnu --target=i686-linux-gnu Thread model: posix gcc version 4.4.5 (Ubuntu/Linaro 4.4.4-14ubuntu5) Don't know why or how, but it did compiled. I'll fix again and send it.

...

+static virInterfacePtr +phypInterfaceDefineXML(virConnectPtr conn, const char *xml, + unsigned int flags) +{

...

+ /* Now adding the new network interface */ + virBufferAddLit(&buf, "chhwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype eth" + " -p %s -o a -s %d -a port_vlan_id=1," + "ieee_virtual_eth=0", def->name, slot);

So if this succeeds,

...

+ /* Need to sleep a little while to wait for the HMC to + * complete the execution of the command. + * */ + sleep(1); + + /* Getting the new interface name */ + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype slot --level slot" + " |sed '/lpar_name=%s/!d; /slot_num=%d/!d; " + "s/^.*drc_name=//'", def->name, slot);

but this fails, do you need to undo the reservation, or have you just leaked a resource?

I put now a roll-back on this part in case it couldn't get the correct interface name.

...

+ + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + goto err; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd,&exit_status, conn); + + if (exit_status< 0 || ret == NULL) + goto err; + + char_ptr = strchr(ret, '\n'); + + if (char_ptr) + *char_ptr = '\0'; + + if (memcpy(name, ret, PHYP_IFACENAME_SIZE-1) == NULL) + goto err;

Huh? memcpy() can never fail on valid input. No need for this if statement; just do the memcpy() and ignore the result (which will be name). Multiple times in this patch.

Fixed.

...

+ +static virInterfacePtr +phypInterfaceLookupByName(virConnectPtr conn, const char *name) +{

...

+ + ret = phypExec(session, cmd,&exit_status, conn); + + if (exit_status< 0 || ret == NULL) + goto err; + + if (memcpy(mac, ret, PHYP_MAC_SIZE-1) == NULL) + goto err; + + VIR_FREE(cmd); + return virGetInterface(conn, name, ret);

Memory leak; you never freed ret. You need to do something like:

result = virGetInterface(conn, name, ret); VIR_FREE(ret); return result;

Fixed

...

+static int +phypInterfaceIsActive(virInterfacePtr iface) +{

...

+ + if (virStrToLong_i(ret,&char_ptr, 10,&state) == -1) + goto err; + + if (char_ptr) + *char_ptr = '\0'; + + VIR_FREE(cmd); + VIR_FREE(ret); + return state;

The lines involving setting *char_ptr to '\0' are useless, and can safely be deleted. No need to NUL-terminate the integer portion of ret if you are just about to free it.

Also fixed.

Getting closer, but still not ready to merge in. Hopefully v5 will clinch it.

Will send a patch right a way. I'll review and test a lot better before sending it. Hope this time we can push it. :-) Thank you very much. -- Eduardo Otubo Software Engineer Linux Technology Center IBM Systems & Technology Group Mobile: +55 19 8135 0885 eotubo@linux.vnet.ibm.com

On 02/16/2011 10:38 AM, Eduardo Otubo wrote:

This is the implementation of the previous patch now using virInterface* API. Ended up this patch got much more simpler, smaller and easier to review. Here is some details:

src/phyp/phyp_driver.c | 648 +++++++++++++++++++++++++++++++++++++++++------- 1 files changed, 562 insertions(+), 86 deletions(-)

All right - this version compiles, so we're already off to a better start than v4, but not ready to apply yet.

@@ -1113,8 +1116,10 @@ openSSHSession(virConnectPtr conn, virConnectAuthPtr auth,

static virDrvOpenStatus phypOpen(virConnectPtr conn, - virConnectAuthPtr auth, int flags ATTRIBUTE_UNUSED) + virConnectAuthPtr auth, int flags) { + virCheckFlags(0, VIR_DRV_OPEN_DECLINED); + LIBSSH2_SESSION *session = NULL;

This introduces a declaration-after-statement. We require C99 for other reasons, therefore this will still compile, but we still like to stick with C89 decl-before-statement where it makes sense; that is, virCheckFlags is usually delayed until after all the declarations.

ConnectionData *connection_data = NULL; char *string = NULL; @@ -1125,6 +1130,7 @@ phypOpen(virConnectPtr conn, char *char_ptr; char *managed_system = NULL;

+ if (!conn || !conn->uri)

Why the spurious whitespace change?

return VIR_DRV_OPEN_DECLINED;

@@ -1176,9 +1182,6 @@ phypOpen(virConnectPtr conn, * */ char_ptr = strchr(managed_system, '/');

- if (char_ptr) - *char_ptr = '\0'; -

This hunk is a regression. Later on, you assign managed_system to phyp_driver->managed_system; without this hunk, that assignment will not contain any /, but after this patch, you've changed what gets assigned.

@@ -1354,11 +1357,6 @@ phypGetLparNAME(LIBSSH2_SESSION * session, const char *managed_system, if (exit_status < 0 || ret == NULL) goto err;

- char *char_ptr = strchr(ret, '\n'); - - if (char_ptr) - *char_ptr = '\0'; - VIR_FREE(cmd); return ret;

This hunk is unrelated to network interface management; and it also looks like a regression (you're now returning a newline and subsequent text, where before-hand you were not). Could you please separate this into multiple patches, each touching one smaller issue, rather than sending one giant patch that mixes both cleanups of existing code and addition of new code? Hint: 'git reset HEAD^; git add -p'.

@@ -1650,9 +1639,6 @@ phypGetBackingDevice(virConnectPtr conn, const char *managed_system,

char_ptr = strchr(backing_device, '\n');

- if (char_ptr) - *char_ptr = '\0'; - VIR_FREE(cmd); VIR_FREE(ret); return backing_device;

Another regression. I'll quit pointing them out, and instead focus on the new code for the rest of my review.

@@ -3273,6 +3223,542 @@ phypGetStoragePoolXMLDesc(virStoragePoolPtr pool, unsigned int flags) }

static int +phypInterfaceDestroy(virInterfacePtr iface, + unsigned int flags) +{ + virCheckFlags(0, -1);

Again, this line belongs...

+ + ConnectionData *connection_data = iface->conn->networkPrivateData; + phyp_driverPtr phyp_driver = iface->conn->privateData; + LIBSSH2_SESSION *session = connection_data->session; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *managed_system = phyp_driver->managed_system; + int system_type = phyp_driver->system_type; + int exit_status = 0; + int slot_num = 0; + int lpar_id = 0; + char *char_ptr; + char *cmd = NULL; + char *ret = NULL;

...here, after declarations.

+ + /* Getting the remote slot number */ + + char_ptr = strchr(iface->mac, '\n');

Dead assignment - you don't use this value of char_ptr for anything...

+ + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype eth --level lpar " + " -F mac_addr,slot_num|" + " sed -n '/%s/ s/^.*,//p'", iface->mac); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + goto err; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, iface->conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &slot_num) == -1)

...before overwriting it here. As an idea for a separate cleanup patch, you reuse a particular idiom of constructing a command in a virBuffer, then checking if that buffer worked, then calling phypExec on the string conversion. Would it be worth a refactoring that changes phypExec from taking a char* to instead taking a virBuffer, to put the error checking in just one place and make all other callers use less code? ...

+ virBufferVSprintf(&buf, + " -r virtualio --rsubtype eth" + " --id %d -o r -s %d", lpar_id, slot_num); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, iface->conn); + + if (exit_status < 0 || ret != NULL) + goto err;

Normally, your idiom has been to declare error if ret == NULL, but here you reversed the logic. Is this a command where you really expect a NULL return, or does phypExec return "" when the command otherwise had no output in which case your logic is backwards?

+static virInterfacePtr +phypInterfaceDefineXML(virConnectPtr conn, const char *xml, + unsigned int flags) +{

+ ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1) + goto err; + + /* The next free slot itself: */ + slot++; + + /* Now adding the new network interface */ + virBufferAddLit(&buf, "chhwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype eth" + " -p %s -o a -s %d -a port_vlan_id=1," + "ieee_virtual_eth=0", def->name, slot); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + goto err; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn);

Mem leak. You assigned to ret twice without an intermediate VIR_FREE().

+ + if (exit_status < 0 || ret != NULL) + goto err; + + /* Need to sleep a little while to wait for the HMC to + * complete the execution of the command. + * */ + sleep(1);

Is a usleep for a few milliseconds sufficient, or does it have to be for the entire second? Is this racy, where a system under high load will need longer than a second?

+ + /* Getting the new interface name */ + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype slot --level slot" + " |sed '/lpar_name=%s/!d; /slot_num=%d/!d; " + "s/^.*drc_name=//'", def->name, slot); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + goto err; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn);

Another leak of ret.

+ + if (exit_status < 0 || ret == NULL){

Formatting nit: s/){/) {/

+ /* roll back and excluding interface if error*/ + VIR_FREE(cmd); + VIR_FREE(ret); + + virBufferAddLit(&buf, "chhwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype eth" + " -p %s -o r -s %d", def->name, slot); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + goto err; + } + goto err; + } + + char_ptr = strchr(ret, '\n'); + + memcpy(name, ret, PHYP_IFACENAME_SIZE-1);

This doesn't guarantee that name is NUL-terminated (and it's wasteful if name is much shorter than PHYP_IFACENAME_SIZE). Are you sure that's okay...

+ + /* Getting the new interface mac addr */ + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + "-r virtualio --rsubtype eth --level lpar " + " |sed '/lpar_name=%s/!d; /slot_num=%d/!d; " + "s/^.*mac_addr=//'", def->name, slot); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + goto err; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + memcpy(mac, ret, PHYP_MAC_SIZE-1); + + VIR_FREE(cmd); + VIR_FREE(ret); + return virGetInterface(conn, name, mac);

...if you get here with a non-NUL-terminated name?

+static virInterfacePtr +phypInterfaceLookupByName(virConnectPtr conn, const char *name) +{

+ ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1) + goto err; + + /*Getting the lpar_id for the interface */ + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype slot --level slot " + " -F drc_name,lpar_id |" + " sed -n '/%s/ s/^.*,//p'", name); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return NULL; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn);

Another leak of ret. I'm starting to feel a bit frustrated that I've pointed this out in previous rounds of reviews, and you still haven't fixed the issues.

+ + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &lpar_id) == -1) + goto err; + + /*Getting the interface mac */ + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype eth --level lpar " + " -F lpar_id,slot_num,mac_addr|" + " sed -n '/%d,%d/ s/^.*,//p'", lpar_id, slot); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return NULL; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn);

Another leak of ret. More of the same through the rest of the patch. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

This is the implementation of the previous patch now using virInterface* API. Ended up this patch got much more simpler, smaller and easier to review. Here is some details: * MAC size and interface name are fixed due to specifications on HMC, both are created automatically and CAN'T be specified from user. They have the following format: * MAC: 122980003002 * Interface name: U9124.720.067BE8B-V3-C0 * I did replaced all the |grep|sed following the comments Eric Blake did on the last patch. * According to my last email, It's not possible to create a network interface without assigning it to a specific lpar. Then, I am using this very minimalistic XML file for testing: <interface type='ethernet' name='LPAR01'> </interface> In this file I am using "name" as the lpar name which I am going to assign the new network interface. I couldn't find a better way to refer to it. Comments are welcome. * Regarding the fact I am sleeping one second waiting for the HMC to complete creation of the interface, I don't have means to check if the whole process is done. All I do is execute a command, wait until is complete (which is not enough in this case) check the return and the exit status. The process of actually creating a networking interface seems to take a little longer than just the return of the ssh control. --- src/phyp/phyp_driver.c | 583 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 564 insertions(+), 19 deletions(-) diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 51f9ff6..b8e7b2f 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -59,6 +59,7 @@ #include "storage_conf.h" #include "nodeinfo.h" #include "files.h" +#include "interface_conf.h" #include "phyp_driver.h" @@ -74,6 +75,8 @@ static unsigned const int HMC = 0; static unsigned const int IVM = 127; +static unsigned const int PHYP_IFACENAME_SIZE = 24; +static unsigned const int PHYP_MAC_SIZE= 12; static int waitsocket(int socket_fd, LIBSSH2_SESSION * session) @@ -3273,6 +3276,552 @@ phypGetStoragePoolXMLDesc(virStoragePoolPtr pool, unsigned int flags) } static int +phypInterfaceDestroy(virInterfacePtr iface, + unsigned int flags) +{ + virCheckFlags(0, -1); + + ConnectionData *connection_data = iface->conn->networkPrivateData; + phyp_driverPtr phyp_driver = iface->conn->privateData; + LIBSSH2_SESSION *session = connection_data->session; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *managed_system = phyp_driver->managed_system; + int system_type = phyp_driver->system_type; + int exit_status = 0; + int slot_num = 0; + int lpar_id = 0; + char *char_ptr; + char *cmd = NULL; + char *ret = NULL; + + /* Getting the remote slot number */ + + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype eth --level lpar " + " -F mac_addr,slot_num|" + " sed -n '/%s/ s/^.*,//p'", iface->mac); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + goto err; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, iface->conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &slot_num) == -1) + goto err; + + /* Getting the remote slot number */ + VIR_FREE(cmd); + VIR_FREE(ret); + + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype eth --level lpar " + " -F mac_addr,lpar_id|" + " sed -n '/%s/ s/^.*,//p'", iface->mac); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + goto err; + } + cmd = virBufferContentAndReset(&buf); + + VIR_FREE(ret); + + ret = phypExec(session, cmd, &exit_status, iface->conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &lpar_id) == -1) + goto err; + + /* excluding interface */ + VIR_FREE(cmd); + VIR_FREE(ret); + + virBufferAddLit(&buf, "chhwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype eth" + " --id %d -o r -s %d", lpar_id, slot_num); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + VIR_FREE(ret); + + ret = phypExec(session, cmd, &exit_status, iface->conn); + + if (exit_status < 0 || ret != NULL) + goto err; + + VIR_FREE(cmd); + VIR_FREE(ret); + return 0; + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return -1; +} + +static virInterfacePtr +phypInterfaceDefineXML(virConnectPtr conn, const char *xml, + unsigned int flags) +{ + virCheckFlags(0, NULL); + + ConnectionData *connection_data = conn->networkPrivateData; + phyp_driverPtr phyp_driver = conn->privateData; + LIBSSH2_SESSION *session = connection_data->session; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *managed_system = phyp_driver->managed_system; + int system_type = phyp_driver->system_type; + int exit_status = 0; + char *char_ptr; + char *cmd = NULL; + int slot = 0; + char *ret = NULL; + char name[PHYP_IFACENAME_SIZE]; + char mac[PHYP_MAC_SIZE]; + virInterfaceDefPtr def; + + if (!(def = virInterfaceDefParseString(xml))) + goto err; + + /* Now need to get the next free slot number */ + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype slot --level slot" + " -Fslot_num --filter lpar_names=%s" + " |sort|tail -n 1", def->name); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + goto err; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1) + goto err; + + /* The next free slot itself: */ + slot++; + + /* Now adding the new network interface */ + virBufferAddLit(&buf, "chhwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype eth" + " -p %s -o a -s %d -a port_vlan_id=1," + "ieee_virtual_eth=0", def->name, slot); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + goto err; + } + cmd = virBufferContentAndReset(&buf); + + VIR_FREE(ret); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret != NULL) + goto err; + + /* Need to sleep a little while to wait for the HMC to + * complete the execution of the command. + * */ + sleep(1); + + /* Getting the new interface name */ + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype slot --level slot" + " |sed '/lpar_name=%s/!d; /slot_num=%d/!d; " + "s/^.*drc_name=//'", def->name, slot); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + goto err; + } + cmd = virBufferContentAndReset(&buf); + + VIR_FREE(ret); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) { + /* roll back and excluding interface if error*/ + VIR_FREE(cmd); + VIR_FREE(ret); + + virBufferAddLit(&buf, "chhwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype eth" + " -p %s -o r -s %d", def->name, slot); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + goto err; + } + goto err; + } + + memcpy(name, ret, PHYP_IFACENAME_SIZE-1); + + /* Getting the new interface mac addr */ + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + "-r virtualio --rsubtype eth --level lpar " + " |sed '/lpar_name=%s/!d; /slot_num=%d/!d; " + "s/^.*mac_addr=//'", def->name, slot); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + goto err; + } + cmd = virBufferContentAndReset(&buf); + + VIR_FREE(ret); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + memcpy(mac, ret, PHYP_MAC_SIZE-1); + + VIR_FREE(cmd); + VIR_FREE(ret); + return virGetInterface(conn, name, mac); + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return NULL; +} + +static virInterfacePtr +phypInterfaceLookupByName(virConnectPtr conn, const char *name) +{ + ConnectionData *connection_data = conn->networkPrivateData; + phyp_driverPtr phyp_driver = conn->privateData; + LIBSSH2_SESSION *session = connection_data->session; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *managed_system = phyp_driver->managed_system; + int system_type = phyp_driver->system_type; + int exit_status = 0; + char *char_ptr; + char *cmd = NULL; + char *ret = NULL; + int slot = 0; + int lpar_id = 0; + char mac[PHYP_MAC_SIZE]; + + /*Getting the slot number for the interface */ + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype slot --level slot " + " -F drc_name,slot_num |" + " sed -n '/%s/ s/^.*,//p'", name); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return NULL; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1) + goto err; + + /*Getting the lpar_id for the interface */ + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype slot --level slot " + " -F drc_name,lpar_id |" + " sed -n '/%s/ s/^.*,//p'", name); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return NULL; + } + cmd = virBufferContentAndReset(&buf); + + VIR_FREE(ret); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &lpar_id) == -1) + goto err; + + /*Getting the interface mac */ + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype eth --level lpar " + " -F lpar_id,slot_num,mac_addr|" + " sed -n '/%d,%d/ s/^.*,//p'", lpar_id, slot); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return NULL; + } + cmd = virBufferContentAndReset(&buf); + + VIR_FREE(ret); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + memcpy(mac, ret, PHYP_MAC_SIZE-1); + + virInterfacePtr result = virGetInterface(conn, name, ret); + + VIR_FREE(cmd); + VIR_FREE(ret); + return result; + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return NULL; + +} + +static int +phypInterfaceIsActive(virInterfacePtr iface) +{ + ConnectionData *connection_data = iface->conn->networkPrivateData; + phyp_driverPtr phyp_driver = iface->conn->privateData; + LIBSSH2_SESSION *session = connection_data->session; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *managed_system = phyp_driver->managed_system; + int system_type = phyp_driver->system_type; + int exit_status = 0; + int state = 0; + char *char_ptr; + char *cmd = NULL; + char *ret = NULL; + + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype eth --level lpar " + " -F mac_addr,state |" + " sed -n '/%s/ s/^.*,//p'", iface->mac); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, iface->conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &state) == -1) + goto err; + + VIR_FREE(cmd); + VIR_FREE(ret); + return state; + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return -1; + +} + +static int +phypListInterfaces(virConnectPtr conn, char **const names, int nnames) +{ + ConnectionData *connection_data = conn->networkPrivateData; + phyp_driverPtr phyp_driver = conn->privateData; + LIBSSH2_SESSION *session = connection_data->session; + int system_type = phyp_driver->system_type; + char *managed_system = phyp_driver->managed_system; + int vios_id = phyp_driver->vios_id; + int exit_status = 0; + int got = 0; + int i; + char *cmd = NULL; + char *ret = NULL; + char *networks = NULL; + char *char_ptr2 = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + virBufferAddLit(&buf, "lshwres"); + if (system_type == HMC) + virBufferVSprintf(&buf, " -m %s", managed_system); + virBufferVSprintf(&buf, " -r virtualio --rsubtype slot --level slot|" + " sed '/eth/!d; /lpar_id=%d/d; s/^.*drc_name=//g'", + vios_id); + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + goto err; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + /* I need to parse the textual return in order to get the network interfaces */ + if (exit_status < 0 || ret == NULL) + goto err; + + networks = ret; + + while (got < nnames) { + char_ptr2 = strchr(networks, '\n'); + + if (char_ptr2) { + *char_ptr2 = '\0'; + if ((names[got++] = strdup(networks)) == NULL) { + virReportOOMError(); + goto err; + } + char_ptr2++; + networks = char_ptr2; + } else + break; + } + + VIR_FREE(cmd); + VIR_FREE(ret); + return got; + + err: + for (i = 0; i < got; i++) + VIR_FREE(names[i]); + VIR_FREE(cmd); + VIR_FREE(ret); + return -1; +} + +static int +phypNumOfInterfaces(virConnectPtr conn) +{ + ConnectionData *connection_data = conn->networkPrivateData; + phyp_driverPtr phyp_driver = conn->privateData; + LIBSSH2_SESSION *session = connection_data->session; + char *managed_system = phyp_driver->managed_system; + int system_type = phyp_driver->system_type; + int vios_id = phyp_driver->vios_id; + int exit_status = 0; + int nnets = 0; + char *char_ptr; + char *cmd = NULL; + char *ret = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + "-r virtualio --rsubtype eth --level lpar|" + "grep -v lpar_id=%d|grep -c lpar_name", vios_id); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &nnets) == -1) + goto err; + + VIR_FREE(cmd); + VIR_FREE(ret); + return nnets; + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return -1; + +} + +static int phypGetLparState(virConnectPtr conn, unsigned int lpar_id) { ConnectionData *connection_data = conn->networkPrivateData; @@ -3795,6 +4344,7 @@ static virDomainPtr phypDomainCreateAndStart(virConnectPtr conn, const char *xml, unsigned int flags) { + virCheckFlags(0, NULL); ConnectionData *connection_data = conn->networkPrivateData; LIBSSH2_SESSION *session = connection_data->session; @@ -4098,27 +4648,22 @@ static virStorageDriver phypStorageDriver = { .poolIsPersistent = NULL }; -static virNetworkDriver phypNetworkDriver = { +static virInterfaceDriver phypInterfaceDriver = { .name = "PHYP", .open = phypVIOSDriverOpen, .close = phypVIOSDriverClose, - .numOfNetworks = NULL, - .listNetworks = NULL, - .numOfDefinedNetworks = NULL, - .listDefinedNetworks = NULL, - .networkLookupByUUID = NULL, - .networkLookupByName = NULL, - .networkCreateXML = NULL, - .networkDefineXML = NULL, - .networkUndefine = NULL, - .networkCreate = NULL, - .networkDestroy = NULL, - .networkDumpXML = NULL, - .networkGetBridgeName = NULL, - .networkGetAutostart = NULL, - .networkSetAutostart = NULL, - .networkIsActive = NULL, - .networkIsPersistent = NULL + .numOfInterfaces = phypNumOfInterfaces, + .listInterfaces = phypListInterfaces, + .numOfDefinedInterfaces = NULL, + .listDefinedInterfaces = NULL, + .interfaceLookupByName = phypInterfaceLookupByName, + .interfaceLookupByMACString = NULL, + .interfaceGetXMLDesc = NULL, + .interfaceDefineXML = phypInterfaceDefineXML, + .interfaceUndefine = NULL, + .interfaceCreate = NULL, + .interfaceDestroy = phypInterfaceDestroy, + .interfaceIsActive = phypInterfaceIsActive }; int @@ -4128,7 +4673,7 @@ phypRegister(void) return -1; if (virRegisterStorageDriver(&phypStorageDriver) < 0) return -1; - if (virRegisterNetworkDriver(&phypNetworkDriver) < 0) + if (virRegisterInterfaceDriver(&phypInterfaceDriver) < 0) return -1; return 0; -- 1.7.1