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 - it is bad practice to move and
rename code at the same time, as it makes diffs harder to review.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
