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;
s/int/size_t/
> + 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
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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list