Re: [Xen-devel] [PATCH for-4.6 5/8] tools/libxl: Save and restore EMULATOR_XENSTORE_DATA content

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Andrew Cooper writes ("Re: [PATCH for-4.6 5/8] tools/libxl: Save and restore 
EMULATOR_XENSTORE_DATA content"):
> On 29/07/15 12:49, Ian Jackson wrote:
> >> +    rel_start = strlen(xs_path) - 7;
> > This is horrible.
> 
> What do you recommend instead?

I don't see why it is necessary to do something like rel_start at all.

This whole patch could probably be made much simpler with something
like

   static const char *const physmap_entries[] = {
       "start_addr", "size", "name", NULL
   };

and then loop over it a few times.

> > I don't understand why you don't copy the whole subdirectory.  This
> > needs to be explained.
> 
> I am replicating the behaviour of the old libxl__toolstack_save(), not
> redesigning the logic from scratch.  (In an ideal world, all this cruft
> would be limited to qemu alone, which is the sole producer and consumer,
> rather than split across qemu, xenstore and libxl, but that boat has
> long since sailed.)

I guess during the freeze, that is a good explanation.

> > If you do want to copy only some of the keys, this thing where you do
> > tiny bits of the work, each time three times, is a very strange
> > structure.

But this comment stands.

> >> +                size_t sz = strlen(s) + 1;                      \
> >> +                ptr = realloc(buf, len + sz);                   \
> >> +                if (!ptr) { rc = ERROR_NOMEM; goto out; }       \
> >
> > Please use the gc.  I guess you are going to tell me that you can't
> > use the gc because of the remus memory leak problem.  Do I need to go
> > and retrofit the per-iteration sub-ao myself ?
> 
> Again, because that what older toolstack_save() did, but without the
> buggy use of "ptr = realloc(ptr, ...".

Well, I'm too late now to insist on the sub-ao, because I'm going away
for a week and a bit starting Monday.

So I guess I will have to live with the non-use of the gc.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel