* Jimi Xenidis <jimix@xxxxxxxxxxxxxx> [2007-01-11 16:53]:
>
> Ok there are a few things here.
Thanks for looking this over.
And just to give you the context for the code, I was attempted to
directly port the code from FlatDeviceTree.py.
> On Jan 11, 2007, at 1:42 PM, Ryan Harper wrote:
> >+
> >+ /* XXX: fetch the number of vcpus configured for this domain
> >+ checking that xc_domain_getinfo returns info for exactly 1
> >+ dom */
>
> Hmm, you are doing this, what is the "XXX" for?
I'll remove.
> >+ if (xc_domain_getinfo(xc_handle, domid, 1, &info) != 1) {
> >+ DPRINTF("xc_domain_getinfo() failed, can't determine
> >max_vcpu_id\n");
> >+ rc = -1;
> >+ goto out;
> >+ }
> >+
> >+ /* NB: max_vcpu_id is zero-based */
> >+ nr_vcpus = info.max_vcpu_id + 1;
>
> Not sure what you are actually doing here, we boot all domains UP and
> hotplug the rest, so the devtree will never have more than one CPU node.
The python code supplies the number of vcpus and it is used in a loop
later on that copy's cpu0's device tree node for each vcpu in the guest.
>
> >+
> >+ /* XXX: fetch the current shadow_memory value for this domain */
> Again, why the "XXX"? are you Vin Diesel or Ice Cube?!
I'll remove. Sometimes. Only on Fridays. =)
> >+ op = XEN_DOMCTL_SHADOW_OP_GET_ALLOCATION;
> >+ if (xc_shadow_control(xc_handle, domid, op, NULL, 0,
> >&shadow_mb, 0, NULL) < 0 ) {
> >+ rc = -1;
> >+ goto out;
> >+ }
> >+
> >+ /* build the devtree here */
> >+ DPRINTF("constructing devtree\n");
> >+ if (make_devtree(&root, domid, mem_mb, nr_vcpus, shadow_mb,
> >cmdline) < 0) {
>
> I'd expect make_devtree() to only take one argument and use separate
> ft_* calls to fill in the rest of these params.
You'd rather have xc_linux_build contain a bunch of ft_* class rather
than hide that in make_devtree() ?
> >diff -r 0279229b6845 -r e4fda6c5e7a9 tools/python/xen/xend/image.py
> >--- a/tools/python/xen/xend/image.py Thu Jan 11 13:39:27 2007 -0600
> >+++ b/tools/python/xen/xend/image.py Thu Jan 11 13:39:27 2007 -0600
> >@@ -234,8 +234,6 @@ class PPC_LinuxImageHandler(LinuxImageHa
>
> this patch should make the PPC_LinuxImageHandler class should just go
> away.
>
Right, we have the same xc_linux_build signature now. I'll remove.
> >diff -r 0279229b6845 -r e4fda6c5e7a9 tools/libxc/powerpc64/
> >mk_flatdevtree.c
> >--- /dev/null Thu Jan 01 00:00:00 1970 +0000
> >+++ b/tools/libxc/powerpc64/mk_flatdevtree.c Thu Jan 11 13:39:27
> >2007 -0600
>
> Would I be correct in thinking that this file is trying to bind
> flatdevtree support routines to libc? if so, maybe this should be
> libc_flatdevtree.c or flatdevtree_libc.c?
libc? I'm not following you. I was putting all of the code required to
make the flat tree into its own file. If you'd rather see it in
flatdevtree.c, I'm fine with that. I'd probably rename make_devtree()
to something like ft_make_devtree(). Would you prefer that?
>
> >@@ -0,0 +1,520 @@
> >+/*
> >+ * This program is free software; you can redistribute it and/or
> >modify
> >+ * it under the terms of the GNU General Public License as
> >published by
> >+ * the Free Software Foundation; either version 2 of the License, or
> >+ * (at your option) any later version.
> >+ *
> >+ * This program is distributed in the hope that it will be useful,
> >+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> >+ * GNU General Public License for more details.
> >+ *
> >+ * You should have received a copy of the GNU General Public License
> >+ * along with this program; if not, write to the Free Software
> >+ * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA
> >02111-1307, USA.
> >+ *
> >+ * Copyright IBM Corporation 2007
> >+ *
> >+ * Authors: Ryan Harper <ryanh@xxxxxxxxxx>
> >+ */
> >+
> >+#include <stdio.h>
> >+#include <string.h>
> >+#include <sys/types.h>
> >+#include <sys/dir.h>
> >+#include <sys/stat.h>
> >+#include <stdlib.h>
> >+#include <fcntl.h>
> >+#include <dirent.h>
> >+#include <unistd.h>
> >+#include <libgen.h>
> >+#include <inttypes.h>
> >+#include <math.h>
> >+#include <regex.h>
> >+
> >+#include <xc_private.h> /* for DPRINTF */
> >+
> >+#include "mk_flatdevtree.h"
> >+
> >+static int _readfile(const char *fullpath, void *data, int len)
> this function static the _ prefix is not necessary (and technically
> the system's namespace), if you want a prefix then make it more
> meaningful.
OK.
>
> >+{
> >+ struct stat st;
> >+ size_t rv;
> >+ FILE *f;
>
> Why fopen(3) rather than open(2) since you are reading 1 byte at a time?
I'll use open(2).
> >+ int bytes = 0;
> >+ int rc = -1;
> >+
> >+ if ((f = fopen(fullpath, "r")) == NULL) {
> >+ DPRINTF("failed to open %s", fullpath);
> >+ goto out;
> >+ }
> >+
>
> OMG, PLEASE no goto's return something meaningful also. I could
> _maybe_ see the "goto close", but the "goto out" is insulting.
OK.
>
> >+ if (stat(fullpath, &st) < 0) {
> >+ DPRINTF("failed to stat %s", fullpath);
> >+ goto close;
> >+ }
> Perhaps you should stat first so you can bail out early if !S_ISREG()
> below.
> or even better how about instead of stat() use fstat(fileno(f), &st),
> since you have the file open anyway.
OK.
> >+
> >+ if (st.st_size > len) {
> >+ DPRINTF("file to be read(%s) exceeds buffer len (%d)\n",
> >fullpath, len);
> >+ goto close;
> >+ }
> >+ rv = (int)st.st_size;
>
> hmm, do you really need the cast?
> hmm2, you do not use rv for anything.
IIRC, I'm supposed to be returning the number of bytes read so I know
the length for calling ft_prop().
>
> >+ if (S_ISREG(st.st_mode)) {
> just to be clear, you are not allowing symlinks, this is fine, I'm
> just checking.
I didn't see any symlinks in the device tree.
> >+ while(1) {
> >+ /* bail if buffer is full */
> >+ if (bytes >= len)
> >+ break;
> >+
> >+ /* read a byte at a time */
> >+ rv = fread((void *)data+bytes, 1, 1, f);
> >+
> >+ /* break when fread doesn't return any data */
> >+ if ( rv == 0 )
> >+ break;
> >+
> >+ rc = ++bytes;
> >+ }
> hmm, why is this not just rc = read(fd, data, MIN(len, st.st_size));
OK.
>
> >+ }
> >+
> >+close:
> >+ fclose(f);
> >+out:
> >+ return rc;
> >+}
> >+
> >+static int _copynode(struct ft_cxt *cxt, const char *dirpath,
> >const char *propfilter)
> This is a scary bad function, not sure what it does, but looks
> awfully complex.
> A lot of other comment apply here.
It will recursively duplicate a path in the device tree. This is used
to dupe /proc/device-tree/cpus/<cpuname>@0 into the tree for each vcpu
of the guest. Keep in mind, this is what FlatDeviceTree.py was doing.
If you don't like, you'll have no arguments from me.
> >+static int _find_first_cpu(const char *dirpath, char *cpupath)
> >+{
> >+ char path[MAX_PATH];
> >+ struct dirent *tree;
> >+ struct stat st;
> >+ DIR* dir;
> >+ int found = 0;
> >+ int rc = -1;
> >+
> >+ if (snprintf(path, MAX_PATH, "%s/%s", HOST_PROC_DEVTREE,
> >"cpus") <= 0) {
> >+ DPRINTF("failed to build cpu path\n");
> >+ goto out;
> >+ }
> >+
> >+ if ((dir = opendir(path)) == NULL) {
> >+ DPRINTF("failed to open dir %s", path);
> >+ goto out;
> >+ }
> >+
> >+ while (!found) {
> >+ char node[MAX_PATH];
> >+
> >+ if ((tree = readdir(dir)) == NULL)
> >+ break; /* reached end of directory entries */
> >+
> >+ /* ignore ., .. */
> >+ if (strcmp(tree->d_name,"." ) == 0 || strcmp(tree-
> >>d_name,"..") == 0)
> >+ continue;
> >+
> >+ /* build full path name of the file, for stat() */
> >+ if (snprintf(node, MAX_PATH, "%s/%s", path, tree->d_name)
> ><= 0) {
> >+ DPRINTF("failed to concat %s to %s", path, tree->d_name);
> >+ goto out;
> >+ }
> >+
> >+ /* stat the entry */
> >+ if (stat(node, &st) < 0) {
> >+ DPRINTF("failed to stat %s\n", node);
> >+ goto out;
> Should you not continue here? It just means someone removed the file
> on you.
I guess it seems broken to me to have a file device tree just disappear.
Continue will work.
> >+ }
> >+
> >+ /* for each dir, check the device_type until we find a cpu*/
> >+ if (S_ISDIR(st.st_mode)) {
> >+ char cpu[MAX_PATH];
> >+ char data[BUFSIZE];
> >+ int len;
> >+
> >+ if (snprintf(cpu, MAX_PATH, "%s/%s", node,
> >"device_type") <= 0) {
> >+ DPRINTF("failed to concat %s to %s",
> >node,"device_type");
> >+ goto out;
> >+ }
> >+
> >+ if ((len = _readfile(cpu, data, BUFSIZE)) < 0) {
> s/BUFSIZE/sizeof(data)/
OK
> >+ DPRINTF("failed to read data from file %s\n", cpu);
> >+ goto out;
> >+ }
> >+
> >+ if (data && ((strncmp(data, "cpu", 3) == 0))) {
> How could (data == NULL)?
Probably left over when I was trying to use fgets() which sets your char
stream pointer to NULL on error. I'll remove.
> you should be comparing the whole word "cpu" not just looking for
> strings that start with cpu.
Yep.
> In fact you could
> const char dev_cpu[] = "cpu";
> char data[sizeof(dev_cpu)];
> and save some stack space.
OK.
>
> If you are looking for _any_ CPU node then you are ok, if you are
> looking for the first then you must look for the CPU node that has a
> "reg" property of 0.
I'll fix. The python implementation didn't bother looking for a reg
prop of 0.
Thanks for the review Jimi.
--
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
(512) 838-9253 T/L: 678-9253
ryanh@xxxxxxxxxx
_______________________________________________
Xen-ppc-devel mailing list
Xen-ppc-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-ppc-devel
|
Home