Re: [Xen-devel] [v7][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Chen, Tiejun writes ("Re: [v7][PATCH 11/16] tools/libxl: detect and avoid 
conflicts with RDM"):
> > There are quite a few of these long lines, which should be wrapped.
> > See tools/libxl/CODING_STYLE.
> 
> Sorry I can't found any case to what you're talking.
> 
> So are you saying this?
> 
> if (strategy == LIBXL_RDM_RESERVE_STRATEGY_IGNORE &&
>      !d_config->num_pcidevs)
> Or

Yes, I meant `linewrapped', not to make a wrapper function.  Sorry for
not being clear.

> >> +        d_config->num_rdms = nr_entries;
> >> +        d_config->rdms = libxl__realloc(NOGC, d_config->rdms,
> >> +                        d_config->num_rdms * sizeof(libxl_device_rdm));
> >
> > This code is remarkably similar to a function later on which adds an
> > rdm.  Please can you factor it out.
> 
> Do you mean I should merge them as one as possible?

"Factor it out" means to break out into a separate function (or maybe
a macro or something, but in this case a function is appropriate).  So
in this case take the two sets of similar code, combine them into a
function with appropriate arguments, and then call that function in
both places.

Finding multiple occurrences of very similar code is usually a sign
that refactoring is needed.

> But seems not be possible because we have seveal combinations of these 
> two conditions, strategy = LIBXL_RDM_RESERVE_STRATEGY_HOST and one or 
> pci devices are also passes through.

I'm not saying you need to merge the two conditions, which are indeed
different, but: the work of reallocing the array and filling in the
new entry could be lifted into a function which would be called in
both places.

> >> +        for (j = 0; j < d_config->num_rdms; j++) {
> >> +            if (d_config->rdms[j].start ==
> >> +                         (uint64_t)xrdm[0].start_pfn << XC_PAGE_SHIFT)
> >
> > This construct
> >     (uint64_t)some_pfn << XC_PAGE_SHIFT
> > appears an awful lot.
> >
> > I would prefer it if it were done in an inline function (or maybe a
> > macro).
> 
> Like this?
> 
> #define AAAA(x) ((uint64_t)x << XC_PAGE_SHIFT)

Something like that, although inline functions are normally better if
a macro is not required.  And in this case it isn't, so it should be a
function I think.

> Sorry I can't figure out a good name here :) Any suggestions?

The hypervisor seems to call this `pfn_to_paddr'.

> >> +    ret = libxl__domain_device_construct_rdm(gc, d_config,
> >> +                                             rdm_mem_boundary,
> >> +                                             &args);
> >> +    if (ret) {
> >> +        LOG(ERROR, "checking reserved device memory failed");
> >> +        goto out;
> >> +    }
> >
> > `rc' should be used here rather than `ret'.  (It is unfortunate that
> > this function has poor style already, but it would be best not to make
> > it worse.)
> 
> I can do this but according to tools/libxl/CODING_STYLE, looks I should 
> first post a separate patch to fix this code style issue, and then 
> rebase my patch, right?

You are introducing a new use of `ret' rather than `rc'.  AFAICT the
function already has a mixture, and there is no problem with just
using `rc' here.  So I think you do not need to fix the rest of the
function: simply using `ret' rather than `rc' in your added lines
would result in an arrangement which would be correct, and at least as
conformant to the style guide as before.

(Of course if you want to fix the rest of the function then that would
be very welcome, and then you should do it as a separate patch.
However, at this stage before the codefreeze you probably prefer to
avoid taking on anything which is not completely critical.)

Thanks,
Ian.

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