From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Jackson Subject: Re: [v7][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM Date: Fri, 10 Jul 2015 11:14:36 +0100 Message-ID: <21919.39692.335512.864741@mariner.uk.xensource.com> References: <1436420047-25356-1-git-send-email-tiejun.chen@intel.com> <1436420047-25356-12-git-send-email-tiejun.chen@intel.com> <21918.47614.887667.234848@mariner.uk.xensource.com> <559F39DB.8030106@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <559F39DB.8030106@intel.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: "Chen, Tiejun" Cc: Stefano Stabellini , Wei Liu , Ian Campbell , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org 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.