5:47 a.m.
On 03/03/2017 11:35 AM, Daniel P. Berrange wrote:
On Fri, Mar 03, 2017 at 11:36:25AM +0000, Joao Martins wrote:
>
>
> On 03/02/2017 05:58 PM, Jim Fehlig wrote:
>> Sorry for the review delay.
>>
>> On 02/08/2017 09:44 AM, Joao Martins wrote:
>>> As it could be shared with libxl which now allows channels to
>>> be created. Also changed filename to match others in the same
>>> directory namely to virqemuagent.{h,c}
>>>
>>> Signed-off-by: Joao Martins <joao.m.martins(a)oracle.com>
>>> ---
>>> po/POTFILES.in | 2 +-
>>> src/Makefile.am | 2 +-
>>> src/libvirt_private.syms | 21 +
>>> src/qemu/qemu_agent.c | 2248
------------------------------------------
>>> src/qemu/qemu_agent.h | 123 ---
>>> src/qemu/qemu_domain.h | 2 +-
>>> src/qemu/qemu_driver.c | 2 +-
>>> src/util/virqemuagent.c | 2248
++++++++++++++++++++++++++++++++++++++++++
>>> src/util/virqemuagent.h | 123 +++
>>> tests/qemuagenttest.c | 2 +-
>>> tests/qemumonitortestutils.c | 2 +-
>>> tests/qemumonitortestutils.h | 2 +-
>>> 12 files changed, 2399 insertions(+), 2378 deletions(-)
>>> delete mode 100644 src/qemu/qemu_agent.c
>>> delete mode 100644 src/qemu/qemu_agent.h
>>> create mode 100644 src/util/virqemuagent.c
>>> create mode 100644 src/util/virqemuagent.h
>>
>> I hope others will opine on this change. It seems reasonable to me and I'm
>> surprised the qemu driver only needed tiny changes to accommodate moving all
>> this code.
>
> Yeap, I too was surprised on how the changes were non disruptive wrt to qemu
> driver :)
>
>>
>> [...]
>>> diff --git a/src/util/virqemuagent.h b/src/util/virqemuagent.h
>>> new file mode 100644
>>> index 0000000..2e81020
>>> --- /dev/null
>>> +++ b/src/util/virqemuagent.h
>>> @@ -0,0 +1,123 @@
>>> +/*
>>> + * virqemuagent.h: interaction with QEMU guest agent
>>> + *
>>> + * Copyright (C) 2006-2012 Red Hat, Inc.
>>> + * Copyright (C) 2006 Daniel P. Berrange
>>> + *
>>> + * This library is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU Lesser General Public
>>> + * License as published by the Free Software Foundation; either
>>> + * version 2.1 of the License, or (at your option) any later version.
>>> + *
>>> + * This library is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>>> + * Lesser General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU Lesser General Public
>>> + * License along with this library. If not, see
>>> + * <http://www.gnu.org/licenses/>;.
>>> + *
>>> + * Author: Daniel P. Berrange <berrange(a)redhat.com>
>>> + */
>>> +
>>> +
>>> +#ifndef __QEMU_AGENT_H__
>>> +# define __QEMU_AGENT_H__
>>> +
>>> +# include "internal.h"
>>> +# include "domain_conf.h"
>>
>> ISTR a recent discussion on the list frowning on using code from src/conf in
>> src/util, although virthostdev.h also includes domain_conf.h.
>>
>> BTW, compilation fails here
>>
>> In file included from util/virqemuagent.c:35:0:
>> util/virqemuagent.h:29:26: fatal error: domain_conf.h: No such file or directory
>> # include "domain_conf.h"
>>
> Hmm, will have to fix it for v1. This series were applied on top of commit
> 2841e67 ("qemu: propagate bridge MTU into qemu "host_mtu"
option") and I didn't
> observe compilation issues (neither make {syntax-check,check} IIRC).
>
>>> +
>>> +typedef struct _qemuAgent qemuAgent;
>>> +typedef qemuAgent *qemuAgentPtr;
>>> +
>>> +typedef struct _qemuAgentCallbacks qemuAgentCallbacks;
>>> +typedef qemuAgentCallbacks *qemuAgentCallbacksPtr;
>>> +struct _qemuAgentCallbacks {
>>> + void (*destroy)(qemuAgentPtr mon,
>>> + virDomainObjPtr vm);
>>> + void (*eofNotify)(qemuAgentPtr mon,
>>> + virDomainObjPtr vm);
>>> + void (*errorNotify)(qemuAgentPtr mon,
>>> + virDomainObjPtr vm);
>>> +};
>>> +
>>> +
>>> +qemuAgentPtr qemuAgentOpen(virDomainObjPtr vm,
>>> + const virDomainChrSourceDef *config,
>>> + qemuAgentCallbacksPtr cb);
>>> +
>>> +void qemuAgentClose(qemuAgentPtr mon);
>>
>> Other files in src/util prefix structs and functions with "vir".
I'm not sure
>> how picky folks are about that. If the consensus is towards the "vir"
prefix,
>> perhaps it would be easier done with a follow-up after the move?
> Yeah I noticed the pattern. Though given the big move of code into util making
> as a follow-up patch would easy review perhaps.
It would be desirable to use the vir name prefix here, but that should
certainly be done as a separate patch
BTW I said follow-up when I actually meant it
in the context of this series i.e.
patch 2/5 would rename these functions to be prefixed by vir.
- it is bad practice to move and
rename code at the same time, as it makes diffs harder to review.
/nods
Joao