* Jimi Xenidis <jimix@xxxxxxxxxxxxxx> [2007-01-15 14:53]:
>
> On Jan 15, 2007, at 1:51 PM, Ryan Harper wrote:
>
> >* Jimi Xenidis <jimix@xxxxxxxxxxxxxx> [2007-01-11 16:53]:
> >>
> >>Ok there are a few things here.
> >
> BTW: some of these issues existed in the original python, but they
> are yours now :)
Of course =)
>
> >Respun with fixes:
> >
> >- preserve and return errno where approriate
> >- using open/close and read/write instead of f*
> >- dropped vcpu argument, only fill out one cpu in devtree
> >- dropped regexp requirment, use a null-terminated list of filters
> >- made sure to call closedir()
> >- fixed double-free of bph on error path
> >- fixed static function names
> >- renamed find_first_cpu to find_cpu, we don't care which cpu we find
>
> I believe you _must_ use the the entry that has a reg property of 0.
Is that because the properties we are interested aren't guaranteed to be
present in all cpu nodes?
> >+static int readfile(const char *fullpath, void *data, int len)
> >+{
> >+ struct stat st;
> >+ int saved_errno;
> >+ int rc = -1;
> >+ int fd;
> >+
> >+ if ((fd = open(fullpath, O_RDONLY)) == -1) {
> >+ PERROR("%s: failed to open file %s", __func__, fullpath);
> >+ return -1;
> >+ }
> >+
> >+ if ((rc = fstat(fd, &st)) == -1) {
> >+ PERROR("%s: failed to stat fd %d", __func__, fd);
> >+ goto error;
> >+ }
> >+
> >+ if (S_ISREG(st.st_mode))
> >+ rc = read(fd, data, MIN(len, st.st_size));
> My brain fart, the MIN() is not necessary. you want to read no-more
> than your buffer allows, so just use len and forget about
> st.st_size. This assumes that you are not interested in the case
> where len yields a partial read, are you?
OK. I thought about putting a warning that the payload is larger than
the length of the buffer and truncating.
> >+ *
> >+ * compare @property string to each string in @filter
> >+ *
> >+ * return 1 if @property matches any filter, otherwise 0
> >+ *
> >+ */
> >+static int match(const char *property, const char **filter)
> >+{
> >+ int i;
> >+
> >+ if ((property == NULL) || (filter == NULL) || (*filter == NULL))
> >+ return -1;
> This will get interpreted as a "match" bye its users, I would not
> even bother checking.
It shouldn't since match == 1. But I see what you mean as I used
if ( !match())
> SEGVs are good! :)
WFM. =)
> >+
> >+ for (i=0; filter[i] != NULL; i++) {
> >+ /* compare the filter to property, ignoring NULL
> >terminator */
> >+ if (strncmp(property, filter[i], sizeof(filter[i])-1) == 0)
>
> This function has no clue what the contents of "filter" is so you
> cannot use sizeof().
> Assuming sizeof() worked, it is your intention to match the substring?
Ack! I was thinking strlen() since we are comparing the property to the
full-lenght of the filter string. The -1 is probably my screw up for
using sizeof() instead of strlen().
> >+static int copynode(struct ft_cxt *cxt, const char *dirpath, const
> >char **propfilter)
> >+{
>
> This is totally informational, but I think the blob/fnmatch routines
> may make this code way simpler.
sure and I liked my regexp, but the concern was what sort of libc
functions would be available in Xen when we implement copynode down
there for dom0 devtree construction.
>
> >+ struct dirent *tree;
> >+ struct stat st;
> >+ DIR *dir;
> >+ char fullpath[MAX_PATH];
> >+ char *bname = NULL;
> >+ char *basec = NULL;
> >+ int saved_errno;
> >+
> >+ if ((dir = opendir(dirpath)) == NULL) {
> >+ PERROR("%s: failed to open dir %s", __func__, dirpath);
> >+ return -1;
> >+ }
> >+
> >+ while (1) {
> >+ if ((tree = readdir(dir)) == NULL)
> >+ break; /* reached end of directory entries */
> >+
> >+ /* ignore . and .. */
> >+ if (strcmp(tree->d_name,"." ) == 0 || strcmp(tree-
> >>d_name,"..") == 0)
> >+ continue;
> >+
> >+ /* build full path name of the file, for stat() */
> >+ if (snprintf(fullpath, sizeof(fullpath), "%s/%s", dirpath,
> >tree->d_name) <= 0) {
> snprintf will almost never return -1, what you are really interested
> in is if the result does not fit in the buffer, so the test would be:
> >= sizeof(fullpath).
> To "be complete" you should also check against "!=-1" which means
> that the strlen() of the result would be to bit for an int (hard to
> do that, but possible) :)
OK
> >+int make_devtree(
> >+ struct ft_cxt *root,
> >+ uint32_t domid, uint32_t mem_mb,
> >+ unsigned long shadow_mb,
> >+ const char *bootargs)
> >+{
> >+ struct boot_param_header *bph = NULL;
> >+ uint64_t val[2];
> >+ uint32_t val32[2];
> >+ uint64_t totalmem;
> >+ uint64_t rma_bytes;
> >+ uint64_t remaining;
> >+ uint64_t pft_size;
> >+ int64_t shadow_mb_log;
> >+ char cpupath[MAX_PATH];
> >+ const char *propfilter[] = { "ibm", "linux,", NULL };
> >+ char *cpupath_copy = NULL;
> >+ char *cpuname = NULL;
> >+ int saved_errno;
> >+ int dtb_fd = -1;
> >+ int rma_log;
> >+
> >+ /* initialize bph to prevent double free on error path */
> >+ root->bph = NULL;
> >+
> >+ /* carve out space for bph */
> >+ if ((bph = (struct boot_param_header *)malloc(BPH_SIZE)) ==
> >NULL) {
> >+ PERROR("%s: Failed to malloc bph buffer size", __func__);
> >+ goto error;
> >+ }
> >+
> >+ /* NB: struct ft_cxt root defined at top of file */
> >+ /* root = Tree() */
> >+ ft_begin(root, bph, BPH_SIZE);
> >+
> >+ /* you MUST set reservations BEFORE _starting_the_tree_ */
> >+
> Any ideas what this reservation is for? is it for the flat-devtree
> itself?
Nope.
> >+ /* root.reserve(0x1000000, 0x1000) */
> >+ val[0] = cpu_to_be64((u64) 0x1000000);
> >+ val[1] = cpu_to_be64((u64) 0x1000);
> >+ ft_add_rsvmap(root, val[0], val[1]);
> >+
>
> this value is keyed off of rma_bytes
No idea, just duping reservations that the python code made. Is there
some place I should be getting these values from?
> >+ /* chosen.addprop('cpu', cpu0.get_phandle()) */
> >+ ft_prop_int(root, "cpu", PHANDLE_CPU0);
>
> Instead of defining a static set of phandles, can you have a function
> that hands out a counted value, sorta like:
> cpu0_phandle = new_handle();
>
> that way we don;t have to associate a numerical value to each,
> especially new ones.
OK.
> >+ /* xen = root.addnode('xen') */
> >+ ft_begin_node(root, "xen");
>
> the 0x3ffc00 value is offset from rma_bytes
> >+
> >+ /* xen.addprop('start-info', long(0x3ffc000), long(0x1000)) */
> >+ val[0] = cpu_to_be64((u64) 0x3ffc000);
> >+ val[1] = cpu_to_be64((u64) 0x1000);
> >+ ft_prop(root, "start-info", val, sizeof(val));
What am I missing here?
> >+
> >+ /* memory@1 is all the rest */
> >+ if (remaining > 0)
> >+ {
>
> this really should be "memory@<rma_bytes>"
>
> >+ /* mem = root.addnode('memory@1') */
> >+ ft_begin_node(root, "memory@1");
> >+
OK.
--
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