Re: [XenPPC] [PATCH 3 of 4] [PATCH] Move flat device tree construction from python to libxc for xc_linux_build()

[Ryan Harper <ryanh@xxxxxxxxxx>]

* 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