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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list