From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kamala Narasimhan Subject: Re: [PATCH 2/5] Xl interface change plus changes to code it impacts Date: Mon, 14 Feb 2011 13:30:14 -0500 Message-ID: <4D5974B6.50507@gmail.com> References: <4D5060EB.3060109@gmail.com> <4D52DB1E.6080101@gmail.com> <4D541AC6.6040802@gmail.com> <4D5443E6.8010704@gmail.com> <4D555368.3010504@gmail.com> <4D558A02.6010106@gmail.com> <19801.27199.25918.814669@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <19801.27199.25918.814669@mariner.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Ian Jackson Cc: "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org Ian Jackson wrote: > Kamala Narasimhan writes ("Re: [xen-devel][PATCH 2/5] Xl interface change plus changes to code it impacts"): >> Per suggestion, along with the latest patch is a more detailed >> description to add to the check-in message - > > It looks good to me, thanks, apart from some nits. If you could > address these I'll apply it :-). > >> - if (libxl__blktap_enabled(&gc)) >> + if (libxl__blktap_enabled(&gc) && >> + disk->format != DISK_BACKEND_QDISK) > > Some mistake here surely ? > Of course! >> diff -r 6c22ae0f6540 tools/libxl/libxl.c >> --- a/tools/libxl/libxl.c Fri Feb 11 16:51:44 2011 +0000 >> +++ b/tools/libxl/libxl.c Fri Feb 11 13:48:12 2011 -0500 > ... >> - switch (disk->phystype) { > ... >> + switch (disk->backend) { > > You have introduced these braces { }, but that seems to me to be just > a stylistic change and they are not really needed ? > No, I have switched disk->phystype to disk->backend. >> + libxl__device_physdisk_major_minor(disk->pdev_path, &major, &minor); > > This function can fail. It has two call sites neither of which check > the return value. Perhaps that should be addressed in a separate > patch, as your change here isn't making it worse. So no need to do > anything about this right now if you don't want to. > Yes, it isn't part of what we intended to change. There are other things too with respect to disk configuration option code that requires revisiting. >> - case PHYSTYPE_QCOW2: >> - case PHYSTYPE_VHD: >> + case DISK_BACKEND_TAP: >> + case DISK_BACKEND_QDISK: { > > Once again, this is a stylistic change. These { } are not used > elsewhere in libxl so you should not introduce them. (It would be a > different matter if there were some reason why they are required in a > particular case, eg to introduce a relevant block scope.) > I agree, will change. >> case DSTATE_PHYSPATH: >> - if ( *p == ',' ) { >> + if (*p == ',') { >> int ioemu_len; > > It is good that you fixed up the formatting in code you changed, but > please do not make formatting changes to unchanged lines. > Argg... I was worried you might ask why I didn't fix the style around the code I changed. I have reverted the changes. > We will do a style cleanup after 4.1 is released. > >> - if ( tok + ioemu_len < end && >> + if (tok + ioemu_len < end && > > Once again. Can you make sure your patch doesn't have /any/ unrelated > formatting changes to lines you don't touch, please ? > Yes. I will send a patch momentarily. Kamala