Re: [Xen-devel] [PATCH v19 06/12] libxl/remus: setup and control disk replication for DRBD backends

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Yang Hongyang writes ("[Xen-devel] [PATCH v19 06/12] libxl/remus: setup and 
control disk replication for DRBD backends"):
> This patch adds the machinery required for protecting a guest's
> disk state, when the guest disk uses a DRBD disk backend.
> This patch comprises of two parts:

Hi.  I thought I would start a bit later in the series this time since
I would like to get a first-pass review of the later patches ASAP.
I'm afraid that once again I'm not likely to finish the whole series
before I go away on leave for the whole of next week.

> --- /dev/null
> +++ b/tools/hotplug/Linux/block-drbd-probe
> @@ -0,0 +1,85 @@
> +#! /bin/bash
> +#
> +# Copyright (C) 2014 FUJITSU LIMITED
> +#

Shouldn't this script run with `set -e' ?

> diff --git a/tools/libxl/libxl_remus_disk_drbd.c 
> b/tools/libxl/libxl_remus_disk_drbd.c
> new file mode 100644
> index 0000000..1be08bb
> --- /dev/null
> +++ b/tools/libxl/libxl_remus_disk_drbd.c
> +
> +/*** drbd implementation ***/
> +const int DRBD_SEND_CHECKPOINT = 20;
> +const int DRBD_WAIT_CHECKPOINT_ACK = 30;

These should presumably be obtained from some drbd header file, rather
than restated here ?

> +int init_subkind_drbd_disk(libxl__remus_devices_state *rds)
> +{
> +    STATE_AO_GC(rds->ao);
> +
> +    rds->drbd_probe_script = GCSPRINTF("%s/block-drbd-probe",
> +                                       libxl__xen_script_dir_path());

Shouldn't there be a way of overriding this from the configuration ?

> +/*----- helper functions, for async calls -----*/
> +static void drbd_async_call(libxl__remus_device *dev,
> +                            void func(libxl__remus_device *),
> +                            libxl__ev_child_callback callback)

Maybe I'm misremembering, but I think another part of your series has
some very similar function.  I think this facility should be provided
once.  It should probably expect its user to pass an ev_child*, and
its user's callback (`func') should use CONTAINER_OF to find the
user's actual state.

It probably wants to be in libxl_aoutils.c.

> +    rdd->ackwait = WEXITSTATUS(status);

Can you explain what this ackwait is ?  It seems to be some kind of
boolean.  Perhaps there is a state machine here that needs to be
explained in a comment.


Aside from that I don't think there are any problems with this patch
specifically, although I still need to go through the various state
management again by looking at this and the generic and net patches.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel