Re: [Xen-devel] [PATCH 1 of 4 v2] libxl: Combine device model arg build functions

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2012-11-30 at 16:24 +0000, George Dunlap wrote:
> # HG changeset patch
> # User George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> # Date 1354287957 0
> # Node ID 722da032ac90c0e1a78b1154fa588bf295d1f009
> # Parent  ae6fb202b233af815466055d9f1a635802a50855
> libxl: Combine device model arg build functions
> 
> qemu-traditional and qemu-upstream have some differences in the way
> the arguments need to be passed.  At the moment, this is dealt with by
> having two entirely separate functions, libxl__build_device_model_args_new
> and libxl__build_device_model_args_old.
> 
> However, at least 80% of these are the same; this means that fixes or
> additions to one may not make it into the other.  Furthermore, there
> are some unaccounable differences in implementation.

FWIW:
 1 file changed, 168 insertions(+), 260 deletions(-)

But that doesn't really show how much of the code in the new function is
shared and how much is per-qemu-version. Casting my eye over the code
with the patch applied (i.e. an unscientific estimate) it seems like
most of it is under an if (dm_new) of some sort.

My main concern is that qemu-xen-traditional is frozen and so this code
shouldn't be changing, but by bundling it with the qemu-xen code it may
be subject to churn as new stuff gets added to the new qemu and plumbed
through.

A related concern is that some of the interfaces we use (e.g. "-hda")
are deprecated in favour of more expressive forms (-drive -device et
al), depending on how things go upstream we may want to switch to using
them.

Perhaps a useful compromise would be to pull the common stuff into a
common function but call out to version specific function for the bits
which differ too.

Perhaps factoring into functional areas might help too? e.g.
make_{disk,net,foo}_args, then the decision to combine or not can be
made on a per functional area basis?

> @@ -523,6 +450,9 @@ static char ** libxl__build_device_model
>          abort();
>      }
> 
> +    if (!dm_new)
> +        goto finish;
> +
>      ram_size = libxl__sizekb_to_mb(b_info->max_memkb - b_info->video_memkb);
>      flexarray_append(dm_args, "-m");
>      flexarray_append(dm_args, libxl__sprintf(gc, "%"PRId64, ram_size));
> @@ -585,33 +515,11 @@ static char ** libxl__build_device_model
>              flexarray_append(dm_args, drive);
>          }
>      }
> +finish:

This feels especially unsatisfying...



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