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.
>
> Thanks a lot for working on this feature! It would be cool to get it in the
> 3.2.0 release.
>
> Regards,
> Jim
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
