Re: [libvirt] [PATCH 00/11] Revisit xen driver Coverity cleanup changes
John Ferlan wrote:
On 01/30/2013 01:51 PM, John Ferlan wrote:
> src/xen/xen_hypervisor.c | 220 ++++++++--------------------------
> src/xen/xen_inotify.c | 48 ++++----
> src/xen/xend_internal.c | 303 ++++++++++++++---------------------------------
> src/xen/xm_internal.c | 195 ++++++++++++++----------------
> src/xen/xs_internal.c | 239 +++++++++++--------------------------
> 5 files changed, 325 insertions(+), 680 deletions(-)
>
>
Thanks for the reviews Osier and Jim. Rather than respond to each, I'll
summarize the consistent points. If I should send a v2, let me know.
w/r/t PrivatePtr on one line:
I was trying to go with the 80 column rule. For 99% they could fit on
one line if I removed one extra space. For a couple, spanning 2 lines
kept the 80 columns in effect. In looking at other drivers - I don't
think any of them typecast the *PrivatePtr)<var>->privateData, so I
could remove that too. I left them in only because they were there.
Yeah, good point. And dropping the cast will make all of them fit within
80 columns :).
w/r/t function headers:
I was trying to be consistent amongst all the files and then more in
general with other drivers. I kept to the concept that if a header fits
within 80 columns then don't break up the line. I changed the files
where a header wouldn't fit into 80 columns to be:
function(type arg,
type2 arg2,
type3 arg3,
type4 arg4)
{
}
I like the approach and will strive to use that style going forward.
w/r/t: domain->name:
I wasn't sure with this one, so I had left it in. I removed them.
Sounds good.
There are also checks for "domain->id != -1" - any
thoughts on those?
Those seem to be running or not checks, right? Seems as though the
virDomainObjIsActive() or hvirCheckFlags() is used elsewhere.
Looks like those checks are all done on a virDomainPtr. Probably best to
leave them as is instead of getting a virDomainObjPtr solely for use
with virDomainObjIsActive.
What about priv->handle in xen_hypervisor.c? Seems as though open
will
set it and everyone else checks it. Given how these drivers call between
each other, I'm inclined to leave as it, but I'm not against removing
them either.
I tend to agree with leaving as is. In practice, the hypervisor
subdriver is always opened, and if it fails loading the whole xen driver
fails. But in theory opening it could be skipped since it is conditional
on xenHavePrivilege().
Regards,
Jim