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